PING^1 [PATCH v2] combine: Tweak the condition of last_set invalidation
Kewen.Lin
linkw@linux.ibm.com
Mon Jun 28 07:00:25 GMT 2021
Hi!
I'd like to gentle ping this:
https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572555.html
BR,
Kewen
on 2021/6/11 下午9:16, Kewen.Lin via Gcc-patches wrote:
> Hi Segher,
>
> Thanks for the review!
>
> on 2021/6/10 上午4:17, Segher Boessenkool wrote:
>> Hi!
>>
>> On Wed, Dec 16, 2020 at 04:49:49PM +0800, Kewen.Lin wrote:
>>> Currently we have the check:
>>>
>>> if (!insn
>>> || (value && rsp->last_set_table_tick >= label_tick_ebb_start))
>>> rsp->last_set_invalid = 1;
>>>
>>> which means if we want to record some value for some reg and
>>> this reg got refered before in a valid scope,
>>
>> If we already know it is *set* in this same extended basic block.
>> Possibly by the same instruction btw.
>>
>>> we invalidate the
>>> set of reg (last_set_invalid to 1). It avoids to find the wrong
>>> set for one reg reference, such as the case like:
>>>
>>> ... op regX // this regX could find wrong last_set below
>>> regX = ... // if we think this set is valid
>>> ... op regX
>>
>> Yup, exactly.
>>
>>> But because of retry's existence, the last_set_table_tick could
>>> be set by some later reference insns, but we see it's set due
>>> to retry on the set (for that reg) insn again, such as:
>>>
>>> insn 1
>>> insn 2
>>>
>>> regX = ... --> (a)
>>> ... op regX --> (b)
>>>
>>> insn 3
>>>
>>> // assume all in the same BB.
>>>
>>> Assuming we combine 1, 2 -> 3 sucessfully and replace them as two
>>> (3 insns -> 2 insns),
>>
>> This will delete insn 1 and write the combined result to insns 2 and 3.
>>
>>> retrying from insn1 or insn2 again:
>>
>> Always 2, but your point remains valid.
>>
>>> it will scan insn (a) again, the below condition holds for regX:
>>>
>>> (value && rsp->last_set_table_tick >= label_tick_ebb_start)
>>>
>>> it will mark this set as invalid set. But actually the
>>> last_set_table_tick here is set by insn (b) before retrying, so it
>>> should be safe to be taken as valid set.
>>
>> Yup.
>>
>>> This proposal is to check whether the last_set_table safely happens
>>> after the current set, make the set still valid if so.
>>
>>> Full SPEC2017 building shows this patch gets more sucessful combines
>>> from 1902208 to 1902243 (trivial though).
>>
>> Do you have some example, or maybe even a testcase? :-)
>>
>
> Sorry for the late reply, it took some time to get one reduced case.
>
> typedef struct SA *pa_t;
>
> struct SC {
> int h;
> pa_t elem[];
> };
>
> struct SD {
> struct SC *e;
> };
>
> struct SA {
> struct {
> struct SD f[1];
> } g;
> };
>
> void foo(pa_t *k, char **m) {
> int l, i;
> pa_t a;
> l = (int)a->g.f[5].e;
> i = 0;
> for (; i < l; i++) {
> k[i] = a->g.f[5].e->elem[i];
> m[i] = "";
> }
> }
>
> Baseline is r12-0 and the option is "-O3 -mcpu=power9 -fno-strict-aliasing",
> with this patch, the generated assembly can save two rlwinm s.
>
>>> + /* Record the luid of the insn whose expression involving register n. */
>>> +
>>> + int last_set_table_luid;
>>
>> "Record the luid of the insn for which last_set_table_tick was set",
>> right?
>>
>
> But it can be updated later to one smaller luid, how about the wording like:
>
>
> + /* 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. */
>
>
>>> -static void update_table_tick (rtx);
>>> +static void update_table_tick (rtx, int);
>>
>> Please remove this declaration instead, the function is not used until
>> after its actual definition :-)
>>
>
> Done.
>
>>> @@ -13243,7 +13247,21 @@ update_table_tick (rtx x)
>>> for (r = regno; r < endregno; r++)
>>> {
>>> reg_stat_type *rsp = ®_stat[r];
>>> - rsp->last_set_table_tick = label_tick;
>>> + if (rsp->last_set_table_tick >= label_tick_ebb_start)
>>> + {
>>> + /* Later references should not have lower ticks. */
>>> + gcc_assert (label_tick >= rsp->last_set_table_tick);
>>
>> This should be obvious, but checking it won't hurt, okay.
>>
>>> + /* 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. 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.
>
> I updated the comments to:
>
> + /* Since combination may generate some instructions
> + to replace some foregoing instructions with the
> + references to register r (using register r), we
> + need to make sure we record the first instruction
> + which is using register r, so always update with
> + the lowest luid here. If the given set happens
> + before this recorded earliest reference, the set
> + value should be safe to be used. */
>
>>> @@ -13359,7 +13378,10 @@ record_value_for_reg (rtx reg, rtx_insn *insn, rtx value)
>>>
>>> /* Mark registers that are being referenced in this value. */
>>> if (value)
>>> - update_table_tick (value);
>>> + {
>>> + gcc_assert (insn);
>>> + update_table_tick (value, DF_INSN_LUID (insn));
>>> + }
>>
>> Don't add that assert please. If you really want one it should come
>> right at the start of the function, not 60 lines later :-)
>>
>
> Exactly, fixed.
>
>> Looks good if I understood this correctly :-)
>>
>>
>
> Thanks again, I also updated the comments in func record_value_for_reg,
> the new version is attached.
>
> BR,
> Kewen
> -----
> gcc/ChangeLog:
>
> * combine.c (struct reg_stat_type): New member
> last_set_table_luid.
> (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.
>
More information about the Gcc-patches
mailing list