[PATCH/RFC] combine: Tweak the condition of last_set invalidation

[Kewen.Lin]

Hi Segher,

on 2021/1/22 上午8:30, Segher Boessenkool wrote:
> Hi Ke Wen,
> 
> On Fri, Jan 15, 2021 at 04:06:17PM +0800, Kewen.Lin wrote:
>> on 2021/1/15 上午8:22, Segher Boessenkool wrote:
>>> On Wed, Dec 16, 2020 at 04:49:49PM +0800, Kewen.Lin wrote:
>>>>    ... op regX  // this regX could find wrong last_set below
>>>>    regX = ...   // if we think this set is valid
>>>>    ... op regX
>>>>
>>>> But because of retry's existence, the last_set_table_tick could
>>>
>>> It is not just because of retry: combine can change other insns than
>>> just i2 and i3, too.  And even changing i2 requires this!
>>
>> Ah, thanks for the information!  Here retry is one example for that
>> we can revisit one instruction but meanwhile the stored information
>> for reg reference can be from that instruction after the current
>> one but visited before.
> 
> Yes; and we do not usually revisit just one insn, but everything after
> it as well.  We only need to revisit thos insns that are fed by what
> has changed, but this is a good enough approximation (we never revisit
> very far back).
> 
>>> The whole reg_stat stuff is an ugly hack that does not work well.  For
>>> example, as in your example, some "known" value can be invalidated
>>> before the combination that wants to know that value is tried.
>>>
>>> We need to have this outside of combine, in a dataflow(-like) thing
>>> for example.  This could take the place of REG_EQ* as well probably
>>> (which is good, there are various problems with that as well).
>>
>> Good point, but IIUC we still need to keep updating(tracking)
>> information like what we put into reg_stat stuff, it's not static
>> since as you pointed out above, combine can change i2/i3 etc,
>> we need to update the information for the changes.
> 
> Yes, we should keep it correct all the time, and for any point in the
> code.  It also can be used by other passes, e.g. it can replace all
> REG_EQ* notes, all nonzero_bits and num_sign_bit_copies, and no doubt
> even more things.
> 
>> Anyway, it's not what this patch tries to solve.  :-P
> 
> :-)
> 
>>>> This proposal is to check whether the last_set_table safely happens
>>>> after the current set, make the set still valid if so.
>>>
>>> I don't think this is safe to do like this, unfortunately.  There are
>>> more places that set last_set_invalid (well, one more), so at the very
>>> minimum this needs a lot more justification.
>>
>> Let me try to explain it more.
>> * Background *
>>
>> There are two places which set last_set_invalid to 1. 
>>
>> CASE 1:
> 
> <SNIP>
> 
> Thanks for the in-depth explanation!
> 
> I think this should be postponed to stage 1 though?  Or is there
> anything very urgent in it?
> 

Yeah, I agree that this belongs to stage1, and there isn't anything
urgent about it.  Thanks for all further comments above!


BR,
Kewen