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