Hi, Gentle ping this:
https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572555.html BR, Kewen on 2021/7/15 上午10:00, Kewen.Lin via Gcc-patches wrote: > Hi, > > Gentle ping this: > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572555.html > > BR, > Kewen > > on 2021/6/28 下午3:00, Kewen.Lin via Gcc-patches wrote: >> 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. >>>
