Jeff Law <jeffreya...@gmail.com> 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