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