[PATCH v2] combine: Tweak the condition of last_set invalidation

[Kewen.Lin]

Hi Segher,

Thanks for the review!

on 2021/11/30 上午6:28, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, Jun 11, 2021 at 09:16:21PM +0800, Kewen.Lin wrote:
>>>> +	      /* Should pick up the lowest luid if the references
>>>> +		 are in the same block.  */
>>>> +	      if (label_tick == rsp->last_set_table_tick
>>>> +		  && rsp->last_set_table_luid > insn_luid)
>>>> +		rsp->last_set_table_luid = insn_luid;
>>>
>>> Why?  Is it conservative for the check you will do later?  Please spell
>>> this out, it is crucial!
>>
>> Since later the combinations involving this insn probably make the
>> register be used in one insn sitting ahead (which has smaller luid than
>> the one which was recorded before).  Yes, it's very conservative, this
>> ensure that we always use the luid of the insn which is the first insn
>> using this register in the block.
> 
> Why would that be correct?!
> 

The later check has:

       if (!insn
-          || (value && rsp->last_set_table_tick >= label_tick_ebb_start))
+          || (value && rsp->last_set_table_tick >= label_tick_ebb_start
+              && !(label_tick == rsp->last_set_table_tick
+                   && DF_INSN_LUID (insn) < rsp->last_set_table_luid)))
         rsp->last_set_invalid = 1;

For "label_tick != rsp->last_set_table_tick", it's the same as before.

For "label_tick == rsp->last_set_table_tick", we have the below:

+              if (label_tick == rsp->last_set_table_tick
+                  && rsp->last_set_table_luid > insn_luid)
+                rsp->last_set_table_luid = insn_luid;

It keeps checking and updating with the smallest LUID of the insns which
have the expression involving register n are placed in last_set_value.

The updating here aims to ensure we always the LUID of first INSN which
uses register n (or saying that having one expression involving register n
is placed in last_set_value).

For the first time we set last_set_table_tick for register n, we will also
set last_set_table_luid.  For below case, we record x for LUID.  Assuming
we combining 1,2,x to 2,x and regX is updated to be used in insn2.  Then
the first INSN using regX has become to insn 2.

  ... reg1 // insn 1
  ...
  ... reg2 // insn 2
  ...
  ... regX // insn x
  ...
  regX     // insn y
  ...

Later whether combining moves regX setting upward or not, the LUID which it
compares with is always the updated smallest one (insn 2 here), not the one
which is set at the beginning.  So I think it's conservative.

>> The last_set invalidation is going
>> to catch the case like:
>>
>>    ... regX  // avoid the set used here ...
>>    regX = ...
>>    ...
>>
>> Once we have the smallest luid one of all insns which use register X,
>> any unsafe regX sets should be caught.
> 
> Yes, you invalidate more, but because you put lies in the table :-(
> 

This patch tries to relax some restrictions, it seems there are no lies.  :)
Could you help to explain this comment more?

>> 	* combine.c (struct reg_stat_type): New member
>> 	last_set_table_luid.
> 
> This fits on one line.
> 
>> 	(update_table_tick): Add one argument for insn luid and
>> 	set last_set_table_luid with it, remove its declaration.
>> 	(record_value_for_reg): Adjust the condition to set
>> 	last_set_invalid nonzero.
> 
> These lines break earlier than they should as well.
> 
>> +  /* Record the luid of the insn which uses register n, the insn should
>> +     be the first one using register n in that block of the insn which
>> +     last_set_table_tick was set for.  */
>> +
>> +  int				last_set_table_luid;
> 
> I'm not sure what this variable is for.  The comment says something
> else than the variable name does, and now I don't know what to
> believe :-)
> 
> The name says it is for a SET, the explanation says it is for a USE.
> 

Good point.  :)  For the existing last_set_table_tick,
  /* Record the value of label_tick when an expression involving register n
     is placed in last_set_value.  */

  int				last_set_table_tick;

it seems it has the set in the name too, but "an expression involving
register n " is actually a reference (use) to register n?

How about the below one referring to "last_set_table_tick"?

/* Record the smallest luid of the insns whose expressions involving
   register n are placed in last_set_value, meanwhile the insns locate
   in the same block of the insn which last_set_table_tick was set for.  */


BR,
Kewen