Jeff Law <[email protected]> writes:
> On 1/8/24 09:59, Richard Sandiford wrote:
>> This is a bit of a hopeful stab, but is the problem that recog_data still
>> had the previous contents of insn 3674, and so extract_insn_cached wrongly
>> thought that it doesn't need to do anything? If so, does something like:
>>
>> diff --git a/gcc/recog.cc b/gcc/recog.cc
>> index a6799e3f5e6..8ba63c78179 100644
>> --- a/gcc/recog.cc
>> +++ b/gcc/recog.cc
>> @@ -267,6 +267,8 @@ validate_change_1 (rtx object, rtx *loc, rtx new_rtx,
>> bool in_group,
>> case invalid. */
>> changes[num_changes].old_code = INSN_CODE (object);
>> INSN_CODE (object) = -1;
>> + if (recog_data.insn == object)
>> + recog_data.insn = nullptr;
>> }
>>
>> num_changes++;
>>
>> fix it? I suppose there's an argument that this belongs in whatever code
>> sets INSN_CODE to a new nonnegative value (so recog_level2 for RTL-SSA).
>> But doing it in validate_change_1 seems more robust, since anything
>> calling that function is considering changing the insn code.
> Nope, doesn't help at all.
Yeah, in hindsight it was a dull guess. recog resets recog_data.insn
itself, so doing it here wasn't likely to help.
> I'd briefly put a reset of the INSN_CODE
> and a call to recog_memoized in the costing path of rtl-ssa to see if
> that would allow things to move forward, but it failed miserably.
>
> I'll pass along the .i file separately. Hopefully it'll fail for you
> and you can debug. But given failure depends on stale bits in
> recog_data, it may not.
Thanks. That led me to the following, which seems a bit more plausible
than my first attempt. I'll test it on aarch64-linux-gnu and
x86_64-linux-gnu. Does it look OK?
Richard
insn_info::calculate_cost computes the costs of unchanged insns lazily,
so that we don't waste time costing instructions that we never try to
change. It therefore has to revert any in-progress changes, cost the
original instruction, and then reapply the in-progress changes.
However, doing that temporarily changes the INSN_CODEs, and so
temporarily invalidates any information cached about the insn.
This means that insn_cost can end up looking at stale data,
or can cache data that becomes stale once the in-progress
changes are reapplied.
This could in principle happen for any use of temporarily_undo_changes
and redo_changes. Those functions in turn share a common subroutine,
swap_change, so that seems like the best place to fix this.
gcc/
* recog.cc (swap_change): Invalidate the cached recog_data if it
describes an insn that is being changed.
---
gcc/recog.cc | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/gcc/recog.cc b/gcc/recog.cc
index a6799e3f5e6..56370e40e01 100644
--- a/gcc/recog.cc
+++ b/gcc/recog.cc
@@ -614,7 +614,11 @@ swap_change (int num)
else
std::swap (*changes[num].loc, changes[num].old);
if (changes[num].object && !MEM_P (changes[num].object))
- std::swap (INSN_CODE (changes[num].object), changes[num].old_code);
+ {
+ std::swap (INSN_CODE (changes[num].object), changes[num].old_code);
+ if (recog_data.insn == changes[num].object)
+ recog_data.insn = nullptr;
+ }
}
/* Temporarily undo all the changes numbered NUM and up, with a view
--
2.25.1
