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
