Re: [PATCH] Add a late-combine pass [PR106594]

[Richard Sandiford <richard.sandiford@arm.com>]

Jeff Law <jeffreyalaw@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