| Message ID | jvdk3m$gt7$1@dough.gmane.org |
|---|---|
| State | New |
| Headers | show |
Forgot to mention: this is to fix PR 54154.
Updated changelog:
2012-08-02 Paulo Matos <Paulo.Matos@csr.com>
PR middle-end/54154
* regcprop.c (copy_value): remove check for redundant moves.
* regcprop.c (copy_value): add check for redundant moves,
remove instructions if redundant.
On Thu, Aug 2, 2012 at 12:17 PM, Paulo J. Matos <paulo at matos-sorge dot com> wrote: > Extended regcprop to check and remove for redundant move instructions > resulting from the pass. > > Paulo. > > 2012-08-02 Paulo Matos <Paulo dot Matos at csr dot com> > > * regcprop.c (copy_value): remove check for redundant moves. > * regcprop.c (copy_value): add check for redundant moves, > remove instructions if redundant. > Hello, Thanks for working on this. How did you test this? How do you account for mode differences between the two registers? Please also fix the patch (and the ChangeLog entry) to conform to the GCC coding style requirements. Ciao! Steven
On Thu, Aug 2, 2012 at 12:19 PM, Paulo J. Matos <paulo@matos-sorge.com> wrote: > Forgot to mention: this is to fix PR 54154. > > Updated changelog: > > 2012-08-02 Paulo Matos <Paulo.Matos@csr.com> > > PR middle-end/54154 > > * regcprop.c (copy_value): remove check for redundant moves. > * regcprop.c (copy_value): add check for redundant moves, > remove instructions if redundant. That's in copyprop_hardreg_forward_1 + gcc_assert(dr != sr); + space before (). + FOR_BB_INSNS_SAFE(bb, insn, next) { Likewise. + unsigned int dr = REGNO(SET_DEST(set)); + unsigned int sr = REGNO(SET_SRC(set)); + Likewise. Richard. > > >
Thanks for the comments, I will be sending a new patch and fixed changelog. On 02/08/12 11:27, Richard Guenther wrote: > On Thu, Aug 2, 2012 at 12:19 PM, Paulo J. Matos <paulo@matos-sorge.com> wrote: >> Forgot to mention: this is to fix PR 54154. >> >> Updated changelog: >> >> 2012-08-02 Paulo Matos <Paulo.Matos@csr.com> >> >> PR middle-end/54154 >> >> * regcprop.c (copy_value): remove check for redundant moves. >> * regcprop.c (copy_value): add check for redundant moves, >> remove instructions if redundant. > > That's in copyprop_hardreg_forward_1 > > + gcc_assert(dr != sr); > + > > space before (). > > + FOR_BB_INSNS_SAFE(bb, insn, next) > { > > Likewise. > > + unsigned int dr = REGNO(SET_DEST(set)); > + unsigned int sr = REGNO(SET_SRC(set)); > + > > Likewise. > > Richard. > >> >> >> >
On 02/08/12 11:25, Steven Bosscher wrote: > Hello, > > Thanks for working on this. > > How did you test this? > This patch is for GCC46. The main problem is that I was not able to reproduce it (yet) for any upstream backends. I therefore patched GCC46 and tested our backend (where I can easily reproduce the problem) with our commercial test suites and our own hand-crafted suite. I would be eager to know if you have any suggestion on how to test this with upstream backends even if we can't reproduce it. One way would be to test trunk before and after patch and see if we introduce any testsuite regressions, but it feels slightly useless if this problem doesn't occur with x86 (maybe due to the large amount of registers, or how move rules are described in the backend). > How do you account for mode differences between > the two registers? From what I understand about the code is that a simple move between two registers will always have registers with the same mode, so mode is not a worry. That was also the assumption of the code that was written before which had in copy_value: /* Assert that SRC has been copied to DEST. Adjust the data structures to reflect that SRC contains an older copy of the shared value. */ static void copy_value (rtx dest, rtx src, struct value_data *vd) { unsigned int dr = REGNO (dest); unsigned int sr = REGNO (src); unsigned int dn, sn; unsigned int i; /* ??? At present, it's possible to see noop sets. It'd be nice if this were cleaned up beforehand... */ if (sr == dr) return; > Please also fix the patch (and the ChangeLog entry) to conform to the > GCC coding style requirements. > Richard already sent me a message about that. Those problems were lack of experience with patch submission. Will sort that out and resend. Thanks for your interest in the patch, Paulo Matos > Ciao! > Steven >
--- //depot/bc/main/devHost/gcc46/gcc/gcc/regcprop.c 2011-09-06 14:29:15.000000000 0100 +++ /home/pm18/p4ws/pm18_binutils/bc/main/devHost/gcc46/gcc/gcc/regcprop.c 2011-09-06 14:29:15.000000000 0100 @@ -301,11 +301,8 @@ unsigned int dn, sn; unsigned int i; - /* ??? At present, it's possible to see noop sets. It'd be nice if - this were cleaned up beforehand... */ - if (sr == dr) - return; - + gcc_assert(dr != sr); + /* Do not propagate copies to the stack pointer, as that can leave memory accesses with no scheduling dependency on the stack update. */ if (dr == STACK_POINTER_REGNUM) @@ -734,9 +731,9 @@ copyprop_hardreg_forward_1 (basic_block bb, struct value_data *vd) { bool anything_changed = false; - rtx insn; + rtx insn, next; - for (insn = BB_HEAD (bb); ; insn = NEXT_INSN (insn)) + FOR_BB_INSNS_SAFE(bb, insn, next) { int n_ops, i, alt, predicated; bool is_asm, any_replacements; @@ -755,10 +752,7 @@ insn, vd); } - if (insn == BB_END (bb)) - break; - else - continue; + continue; } set = single_set (insn); @@ -966,10 +960,19 @@ /* Notice copies. */ if (set && REG_P (SET_DEST (set)) && REG_P (SET_SRC (set))) - copy_value (SET_DEST (set), SET_SRC (set), vd); - - if (insn == BB_END (bb)) - break; + { + unsigned int dr = REGNO(SET_DEST(set)); + unsigned int sr = REGNO(SET_SRC(set)); + + if(dr == sr) + { + /* noop set */ + delete_insn_and_edges(insn); + } + else + copy_value (SET_DEST (set), SET_SRC (set), vd); + } + } return anything_changed;