Hi, 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. >>>>>
