Message ID | CAHz1=dVN+C8QJBRW-fnK_LNA3A+P2c79k6ieE4jNrUBQsQzdnA@mail.gmail.com |
---|---|
State | New |
Headers | show |
>OK for mainline? Doh, hard to believe we never checked that an insn defines a register before spitting out reg_moves for it ... nice catch. This + /* Skip instructions that do not set a register. */ + if (set && !REG_P (SET_DEST (set))) + continue; is ok. Can you also prevent !set insns from having reg_moves? (To be updated once auto_inc insns will be supported, if they'll deserve reg_moves too.) Ayal. On Mon, Sep 26, 2011 at 7:31 AM, Revital Eres <revital.eres@linaro.org>wrote: > Hello, > > The attached patch contains a fix to generate_reg_moves > function. Currently we can generate reg-moves for stores which are later > eliminated. This happens when we have mem dependency with distance 1 > and as a result the number of regmoves is at least 1 based on the > following > calculation taken from generate_reg_moves (): > > if (e->distance == 1) > nreg_moves4e = (SCHED_TIME (e->dest) - SCHED_TIME (e->src) + ii) / ii; > > This is an example of register move generated in such cases: > > reg_move = (insn 152 119 75 4 (set (reg:SI 231) > (mem:SI (pre_modify:DI (reg:DI 215) > (plus:DI (reg:DI 215) > (reg:DI 171 [ ivtmp.42 ]))) [3 MEM[base: > pretmp.27_65, index: ivtmp.42_9, offset: 0B]+0 S4 A32])) -1 > (nil)) > > When not handling REG_INC instructions this was not a problem as these > reg-moves were removes by dead code elimination. > for example: > > insn 1) mem[x] = ... > insn 2) .. = mem[y] > > When reg-move reg1 = mem [x] was generated mem[x] is not been used in > insn 2 and thus reg1 could be eliminated. > But with REG_INC this is different because the reg-move instruction > remains and leads to bad gen. > The attached tescase capture this case. > > Tested and bootstrap with patch 2 on ppc64-redhat-linux > enabling SMS on loops with SC 1. On arm-linux-gnueabi bootstrap c on > top of the set of patches that support do-loop pattern > (http://gcc.gnu.org/ml/gcc-patches/2011-07/msg01807.html) which solves > the bootstrap failure on ARM with SMS flags. > > OK for mainline? > > Thanks, > Revital > > gcc/ > * modulo-sched.c (generate_reg_moves): Skip instructions that > do not set a register. > > > testsuite/ > * gcc.dg/sms-10.c: New file. >
>OK for mainline? Doh, hard to believe we never checked that an insn defines a register before spitting out reg_moves for it ... nice catch. This + /* Skip instructions that do not set a register. */ + if (set && !REG_P (SET_DEST (set))) + continue; is ok. Can you also prevent !set insns from having reg_moves? (To be updated once auto_inc insns will be supported, if they'll deserve reg_moves too.) Ayal. > > On Mon, Sep 26, 2011 at 7:31 AM, Revital Eres <revital.eres@linaro.org> wrote: >> >> Hello, >> >> The attached patch contains a fix to generate_reg_moves >> function. Currently we can generate reg-moves for stores which are later >> eliminated. This happens when we have mem dependency with distance 1 >> and as a result the number of regmoves is at least 1 based on the >> following >> calculation taken from generate_reg_moves (): >> >> if (e->distance == 1) >> nreg_moves4e = (SCHED_TIME (e->dest) - SCHED_TIME (e->src) + ii) / ii; >> >> This is an example of register move generated in such cases: >> >> reg_move = (insn 152 119 75 4 (set (reg:SI 231) >> (mem:SI (pre_modify:DI (reg:DI 215) >> (plus:DI (reg:DI 215) >> (reg:DI 171 [ ivtmp.42 ]))) [3 MEM[base: >> pretmp.27_65, index: ivtmp.42_9, offset: 0B]+0 S4 A32])) -1 >> (nil)) >> >> When not handling REG_INC instructions this was not a problem as these >> reg-moves were removes by dead code elimination. >> for example: >> >> insn 1) mem[x] = ... >> insn 2) .. = mem[y] >> >> When reg-move reg1 = mem [x] was generated mem[x] is not been used in >> insn 2 and thus reg1 could be eliminated. >> But with REG_INC this is different because the reg-move instruction >> remains and leads to bad gen. >> The attached tescase capture this case. >> >> Tested and bootstrap with patch 2 on ppc64-redhat-linux >> enabling SMS on loops with SC 1. On arm-linux-gnueabi bootstrap c on >> top of the set of patches that support do-loop pattern >> (http://gcc.gnu.org/ml/gcc-patches/2011-07/msg01807.html) which solves >> the bootstrap failure on ARM with SMS flags. >> >> OK for mainline? >> >> Thanks, >> Revital >> >> gcc/ >> * modulo-sched.c (generate_reg_moves): Skip instructions that >> do not set a register. >> >> >> testsuite/ >> * gcc.dg/sms-10.c: New file. >
Hello, > This > + /* Skip instructions that do not set a register. */ > + if (set && !REG_P (SET_DEST (set))) > + continue; > is ok. Can you also prevent !set insns from having reg_moves? (To be updated > once auto_inc insns will be supported, if they'll deserve reg_moves too.) Do you mean leaving any anti-dep edges to !set instructions similar to what is done for auto_inc addresses in part 2 of this patch? Thanks, Revital
On Tue, Sep 27, 2011 at 10:47 AM, Revital Eres <revital.eres@linaro.org> wrote: > Hello, > >> This >> + /* Skip instructions that do not set a register. */ >> + if (set && !REG_P (SET_DEST (set))) >> + continue; >> is ok. Can you also prevent !set insns from having reg_moves? (To be updated >> once auto_inc insns will be supported, if they'll deserve reg_moves too.) > > Do you mean leaving any anti-dep edges to !set instructions similar to > what is done for auto_inc addresses in part 2 of this patch? > No, I mean have a "if (!set) continue;" right after the above in generate_reg_moves(). For patch 2/2 this would need to be updated to allow generating reg_moves for non-single_set's which are auto-inc's (making sure we generate them for the right reg). Ayal. > Thanks, > Revital >
Index: modulo-sched.c =================================================================== --- modulo-sched.c (revision 179138) +++ modulo-sched.c (working copy) @@ -476,7 +476,12 @@ generate_reg_moves (partial_schedule_ptr sbitmap *uses_of_defs; rtx last_reg_move; rtx prev_reg, old_reg; - + rtx set = single_set (u->insn); + + /* Skip instructions that do not set a register. */ + if (set && !REG_P (SET_DEST (set))) + continue; + /* Compute the number of reg_moves needed for u, by looking at life ranges started at u (excluding self-loops). */ for (e = u->out; e; e = e->next_out)