| Message ID | 20140909115027.GR17693@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 9, 2014 at 1:50 PM, Alan Modra <amodra@gmail.com> wrote: > On Fri, Sep 05, 2014 at 11:00:04AM +0930, Alan Modra wrote: >> Of course it would be better to repair the damage done to debug info >> rather than rejecting it outright.. > > This cures PR60655 on PowerPC by passing the horrible debug_loc > expressions we have through simplify_rtx. Not only do we get > reg10 + &.LANCHOR0 + const(0x14f - &.LANCHOR0) and > reg10 + &modulus + const(~&d_data), > the two expressions that cause the PR due to (CONST (MINUS ..)) and > (CONST (NOT ..)), but also things like > ~reg5 - reg31 + reg5 + reg10 + &modulus > where "~reg5 + reg5" is an inefficient way to write "-1". > > It turns out that merely passing these expression through simplify_rtx > isn't enough. Some tweaking is necessary to make (CONST (NOT ..)) and > (CONST (MINUS ..)) recognised by simplify_plus_minus, and even after > doing that, the two reg5 terms are not cancelled. > > The reg5 case starts off with the simplify_plus_minus sorted ops array > effectively containing the expression > -reg31 + reg10 + reg5 + -reg5 + &modulus + -1 > > The first combination tried is &modulus + -1, which is rejected to > prevent recursion. The next combination tried is -reg5 + -1, which is > simlified to ~reg5. Well, that is a valid simplification, but the > trouble is that this prevents "reg5 + -reg5" being simplified to 0. > What's more, "&modulus + -1" is no more expensive than "&modulus", > since they are both emitted as an address field with a symbol+addend > relocation. For that reason, I believe we should not consider > combining a const_int with any other term after finding that it can be > combined with a symbol_ref or other similar term. > > Bootstrapped and regression tested powerpc64-linux and x86_64-linux. > I measured before/after bootstrap times on x86_64-linux because I was > concerned about the extra simplify_rtx calls, and was pleasantly > surprised to see a 0.23% improvement in bootstrap time. (Which of > course is likely just noise.) OK to apply? > > PR debug/60655 > * simplify-rtx.c (simplify_plus_minus): Delete unused "input_ops". > Handle CONST wrapped NOT, NEG and MINUS. Break out of innermost > loop when finding a trivial CONST expression. > * var-tracking.c (vt_expand_var_loc_chain): Call simplify_rtx. > > Index: gcc/simplify-rtx.c > =================================================================== > --- gcc/simplify-rtx.c (revision 214487) > +++ gcc/simplify-rtx.c (working copy) > @@ -3960,7 +3960,7 @@ simplify_plus_minus (enum rtx_code code, enum mach > { > struct simplify_plus_minus_op_data ops[8]; > rtx result, tem; > - int n_ops = 2, input_ops = 2; > + int n_ops = 2; > int changed, n_constants = 0, canonicalized = 0; > int i, j; > > @@ -3997,7 +3997,6 @@ simplify_plus_minus (enum rtx_code code, enum mach > n_ops++; > > ops[i].op = XEXP (this_op, 0); > - input_ops++; > changed = 1; > canonicalized |= this_neg; > break; > @@ -4010,14 +4009,35 @@ simplify_plus_minus (enum rtx_code code, enum mach > break; > > case CONST: > - if (n_ops < 7 > - && GET_CODE (XEXP (this_op, 0)) == PLUS > - && CONSTANT_P (XEXP (XEXP (this_op, 0), 0)) > - && CONSTANT_P (XEXP (XEXP (this_op, 0), 1))) > + if (GET_CODE (XEXP (this_op, 0)) == NEG > + && CONSTANT_P (XEXP (XEXP (this_op, 0), 0))) > { > ops[i].op = XEXP (XEXP (this_op, 0), 0); > + ops[i].neg = !this_neg; > + changed = 1; > + canonicalized = 1; > + } > + else if (n_ops < 7 > + && GET_CODE (XEXP (this_op, 0)) == NOT > + && CONSTANT_P (XEXP (XEXP (this_op, 0), 0))) > + { > + ops[n_ops].op = CONSTM1_RTX (mode); > + ops[n_ops++].neg = this_neg; > + ops[i].op = XEXP (XEXP (this_op, 0), 0); > + ops[i].neg = !this_neg; > + changed = 1; > + canonicalized = 1; > + } > + else if (n_ops < 7 > + && (GET_CODE (XEXP (this_op, 0)) == PLUS > + || GET_CODE (XEXP (this_op, 0)) == MINUS) > + && CONSTANT_P (XEXP (XEXP (this_op, 0), 0)) > + && CONSTANT_P (XEXP (XEXP (this_op, 0), 1))) > + { > + ops[i].op = XEXP (XEXP (this_op, 0), 0); > ops[n_ops].op = XEXP (XEXP (this_op, 0), 1); > - ops[n_ops].neg = this_neg; > + ops[n_ops].neg > + = (GET_CODE (XEXP (this_op, 0)) == MINUS) ^ this_neg; > n_ops++; > changed = 1; > canonicalized = 1; > @@ -4141,16 +4161,21 @@ simplify_plus_minus (enum rtx_code code, enum mach > else > tem = simplify_binary_operation (ncode, mode, lhs, rhs); > > - /* Reject "simplifications" that just wrap the two > - arguments in a CONST. Failure to do so can result > - in infinite recursion with simplify_binary_operation > - when it calls us to simplify CONST operations. */ > - if (tem > - && ! (GET_CODE (tem) == CONST > - && GET_CODE (XEXP (tem, 0)) == ncode > - && XEXP (XEXP (tem, 0), 0) == lhs > - && XEXP (XEXP (tem, 0), 1) == rhs)) > + if (tem) > { > + /* Reject "simplifications" that just wrap the two > + arguments in a CONST. Failure to do so can result > + in infinite recursion with simplify_binary_operation > + when it calls us to simplify CONST operations. > + Also, if we find such a simplification, don't try > + any more combinations with this rhs: We must have > + something like symbol+offset, ie. one of the > + trivial CONST expressions we handle later. */ > + if (GET_CODE (tem) == CONST > + && GET_CODE (XEXP (tem, 0)) == ncode > + && XEXP (XEXP (tem, 0), 0) == lhs > + && XEXP (XEXP (tem, 0), 1) == rhs) > + break; > lneg &= rneg; > if (GET_CODE (tem) == NEG) > tem = XEXP (tem, 0), lneg = !lneg; > Index: gcc/var-tracking.c > =================================================================== > --- gcc/var-tracking.c (revision 214898) > +++ gcc/var-tracking.c (working copy) > @@ -8375,7 +8375,15 @@ vt_expand_var_loc_chain (variable var, bitmap regs > *pendrecp = pending_recursion; > > if (!pendrecp || !pending_recursion) > - var->var_part[0].cur_loc = result; > + { > + if (result) > + { > + rtx tem = simplify_rtx (result); why wasn't 'result' built using simplify_gen_* in the first place? I also note that debug_insns can have all sorts of weird (even invalid) and un-recognized RTL which the simplify_rtx machinery may not like (and thus ICE). CSELIB seems to do the less optimal copy_rtx, valueize operands, re-simplify operation instead of valueizing the operands and then simplify_gen_ the actual RTX here (skipping that when valueizing the operands did nothing). Richard. > + if (tem) > + result = tem; > + } > + var->var_part[0].cur_loc = result; > + } > > return result; > } > > -- > Alan Modra > Australia Development Lab, IBM
On Tue, Sep 09, 2014 at 04:25:23PM +0200, Richard Biener wrote: > why wasn't 'result' built using simplify_gen_* in the first place? I also It is built using cselib_expand_value_rtx_cb, which calls the various simplify_*_operation and simplify_rtx too. > note that debug_insns can have all sorts of weird (even invalid) and > un-recognized RTL which the simplify_rtx machinery may not like > (and thus ICE). That would be bug in simplify-rtx.c if we ICEd on that. Jakub
On Tue, Sep 9, 2014 at 4:30 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Sep 09, 2014 at 04:25:23PM +0200, Richard Biener wrote: >> why wasn't 'result' built using simplify_gen_* in the first place? I also > > It is built using cselib_expand_value_rtx_cb, which calls the various > simplify_*_operation and simplify_rtx too. So why do we need to simplify again then? >> note that debug_insns can have all sorts of weird (even invalid) and >> un-recognized RTL which the simplify_rtx machinery may not like >> (and thus ICE). > > That would be bug in simplify-rtx.c if we ICEd on that. Ok, fair enough. Richard. > Jakub
On Tue, Sep 09, 2014 at 04:42:04PM +0200, Richard Biener wrote: > On Tue, Sep 9, 2014 at 4:30 PM, Jakub Jelinek <jakub@redhat.com> wrote: > > On Tue, Sep 09, 2014 at 04:25:23PM +0200, Richard Biener wrote: > >> why wasn't 'result' built using simplify_gen_* in the first place? I also > > > > It is built using cselib_expand_value_rtx_cb, which calls the various > > simplify_*_operation and simplify_rtx too. > > So why do we need to simplify again then? It looks to me that cselib_expand_value_rtx_1 doesn't call simplify_rtx for DEBUG_EXPR.
On Tue, 9 Sep 2014, Alan Modra wrote: > This cures PR60655 on PowerPC by passing the horrible debug_loc > expressions we have through simplify_rtx. Not only do we get > reg10 + &.LANCHOR0 + const(0x14f - &.LANCHOR0) and > reg10 + &modulus + const(~&d_data), > the two expressions that cause the PR due to (CONST (MINUS ..)) and > (CONST (NOT ..)), but also things like > ~reg5 - reg31 + reg5 + reg10 + &modulus > where "~reg5 + reg5" is an inefficient way to write "-1". > > It turns out that merely passing these expression through simplify_rtx > isn't enough. Some tweaking is necessary to make (CONST (NOT ..)) and > (CONST (MINUS ..)) recognised by simplify_plus_minus, and even after > doing that, the two reg5 terms are not cancelled. > > The reg5 case starts off with the simplify_plus_minus sorted ops array > effectively containing the expression > -reg31 + reg10 + reg5 + -reg5 + &modulus + -1 > > The first combination tried is &modulus + -1, which is rejected to > prevent recursion. The next combination tried is -reg5 + -1, which is > simlified to ~reg5. Well, that is a valid simplification, but the > trouble is that this prevents "reg5 + -reg5" being simplified to 0. > What's more, "&modulus + -1" is no more expensive than "&modulus", > since they are both emitted as an address field with a symbol+addend > relocation. For that reason, I believe we should not consider > combining a const_int with any other term after finding that it can be > combined with a symbol_ref or other similar term. > > Bootstrapped and regression tested powerpc64-linux and x86_64-linux. > I measured before/after bootstrap times on x86_64-linux because I was > concerned about the extra simplify_rtx calls, and was pleasantly > surprised to see a 0.23% improvement in bootstrap time. (Which of > course is likely just noise.) OK to apply? Thanks for your work on this issue, I have tested your change with my usual powerpc-gnu-linux multilibs with the old and new result for gcc.c-torture/compile/pr60655-2.c noted on the right: -mcpu=603e FAIL -> PASS -mcpu=603e -msoft-float FAIL -> PASS -mcpu=8540 -mfloat-gprs=single -mspe=yes -mabi=spe FAIL -> PASS -mcpu=8548 -mfloat-gprs=double -mspe=yes -mabi=spe FAIL -> PASS -mcpu=7400 -maltivec -mabi=altivec FAIL -> PASS -mcpu=e6500 -maltivec -mabi=altivec FAIL -> PASS -mcpu=e5500 -m64 PASS -> PASS -mcpu=e6500 -m64 -maltivec -mabi=altivec PASS -> PASS -- please note that the test case used to pass for 64-bit multilibs even before your fix so unless your powerpc64-linux configuration includes 32-bit multilibs as well, it does not really provide suitable coverage. Once your fix has been put in place the old ARM hack made for 4.9, that my original proposal has been based on: 2014-04-10 Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> PR debug/60655 * config/arm/arm.c (TARGET_CONST_NOT_OK_FOR_DEBUG_P): Define (arm_const_not_ok_for_debug_p): Reject MINUS with SYM_REF's ameliorating the cases where it can be. can I suppose be reverted too. Maciej
On Wed, Sep 10, 2014 at 01:43:22PM +0100, Maciej W. Rozycki wrote: > Thanks for your work on this issue, I have tested your change with my > usual powerpc-gnu-linux multilibs with the old and new result for > gcc.c-torture/compile/pr60655-2.c noted on the right: > > -mcpu=603e FAIL -> PASS > -mcpu=603e -msoft-float FAIL -> PASS > -mcpu=8540 -mfloat-gprs=single -mspe=yes -mabi=spe FAIL -> PASS > -mcpu=8548 -mfloat-gprs=double -mspe=yes -mabi=spe FAIL -> PASS > -mcpu=7400 -maltivec -mabi=altivec FAIL -> PASS > -mcpu=e6500 -maltivec -mabi=altivec FAIL -> PASS > -mcpu=e5500 -m64 PASS -> PASS > -mcpu=e6500 -m64 -maltivec -mabi=altivec PASS -> PASS > > -- please note that the test case used to pass for 64-bit multilibs even > before your fix so unless your powerpc64-linux configuration includes > 32-bit multilibs as well, it does not really provide suitable coverage. I always build powerpc64-linux with --enable-targets=powerpc-linux and regression test with RUNTESTFLAGS=--target_board=unix/'{-m32,-m64}', so my "bootstrapped and regression tested powerpc64-linux" claim includes a -m32 regression test too. Not quite as comprehensive a test as you've done (thanks!), but I did see the FAIL->PASS for -m32.
On Wed, 10 Sep 2014, Alan Modra wrote: > I always build powerpc64-linux with --enable-targets=powerpc-linux and > regression test with RUNTESTFLAGS=--target_board=unix/'{-m32,-m64}', > so my "bootstrapped and regression tested powerpc64-linux" claim > includes a -m32 regression test too. Not quite as comprehensive a > test as you've done (thanks!), but I did see the FAIL->PASS for -m32. OK, good, I just wanted to be sure we're on the same page. Thanks! Maciej
> > 2014-04-10 Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> > > PR debug/60655 > * config/arm/arm.c (TARGET_CONST_NOT_OK_FOR_DEBUG_P): Define > (arm_const_not_ok_for_debug_p): Reject MINUS with SYM_REF's > ameliorating the cases where it can be. > > can I suppose be reverted too. That was always something to help get 4.9 out of the door - I still mean to do the proper work for 5.0 where we look at all possible types of output generated but it won't happen this month either. If this fixes it properly it would be nice to revert the change - I can't take this on for a few weeks as I am travelling. If you want to take that up please verify that the original testcase is fixed and the usual caveats of testing apply. Thanks, Ramana > > Maciej >
On Wed, 10 Sep 2014, Ramana Radhakrishnan wrote: > > 2014-04-10 Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> > > > > PR debug/60655 > > * config/arm/arm.c (TARGET_CONST_NOT_OK_FOR_DEBUG_P): Define > > (arm_const_not_ok_for_debug_p): Reject MINUS with SYM_REF's > > ameliorating the cases where it can be. > > > > can I suppose be reverted too. > > That was always something to help get 4.9 out of the door - I still mean to do > the proper work for 5.0 where we look at all possible types of output > generated but it won't happen this month either. > > If this fixes it properly it would be nice to revert the change - I can't take > this on for a few weeks as I am travelling. If you want to take that up please > verify that the original testcase is fixed and the usual caveats of testing > apply. This is supposed to properly fix the issue the test case covers I believe. I'm not going to do anything for the ARM target here, I just thought you might want to know that PR debug/60655 has now been further addressed, as it may be easy to miss mailing list traffic (especially as ARM has been only sparsely mentioned in the discussion). And what you do with this knowledge is up to you. :) Maciej
Ping? https://gcc.gnu.org/ml/gcc-patches/2014-09/msg00704.html
On Thu, Oct 16, 2014 at 05:25:57PM +1030, Alan Modra wrote: > Ping? > https://gcc.gnu.org/ml/gcc-patches/2014-09/msg00704.html I think the simplification should be done when constructing the expressions, i.e. if possible in the simplification callback or so if it isn't performed at some level. Because otherwise, you construct the RTL all the way up into complex expressions, and then another simplification will, if there are simplifications e.g. very deep in the expressions, copy the rest all the way up, creating tons of GC garbage. So, please find the spot where we forget to simplify stuff, and put the simplification there. Jakub
Index: gcc/simplify-rtx.c =================================================================== --- gcc/simplify-rtx.c (revision 214487) +++ gcc/simplify-rtx.c (working copy) @@ -3960,7 +3960,7 @@ simplify_plus_minus (enum rtx_code code, enum mach { struct simplify_plus_minus_op_data ops[8]; rtx result, tem; - int n_ops = 2, input_ops = 2; + int n_ops = 2; int changed, n_constants = 0, canonicalized = 0; int i, j; @@ -3997,7 +3997,6 @@ simplify_plus_minus (enum rtx_code code, enum mach n_ops++; ops[i].op = XEXP (this_op, 0); - input_ops++; changed = 1; canonicalized |= this_neg; break; @@ -4010,14 +4009,35 @@ simplify_plus_minus (enum rtx_code code, enum mach break; case CONST: - if (n_ops < 7 - && GET_CODE (XEXP (this_op, 0)) == PLUS - && CONSTANT_P (XEXP (XEXP (this_op, 0), 0)) - && CONSTANT_P (XEXP (XEXP (this_op, 0), 1))) + if (GET_CODE (XEXP (this_op, 0)) == NEG + && CONSTANT_P (XEXP (XEXP (this_op, 0), 0))) { ops[i].op = XEXP (XEXP (this_op, 0), 0); + ops[i].neg = !this_neg; + changed = 1; + canonicalized = 1; + } + else if (n_ops < 7 + && GET_CODE (XEXP (this_op, 0)) == NOT + && CONSTANT_P (XEXP (XEXP (this_op, 0), 0))) + { + ops[n_ops].op = CONSTM1_RTX (mode); + ops[n_ops++].neg = this_neg; + ops[i].op = XEXP (XEXP (this_op, 0), 0); + ops[i].neg = !this_neg; + changed = 1; + canonicalized = 1; + } + else if (n_ops < 7 + && (GET_CODE (XEXP (this_op, 0)) == PLUS + || GET_CODE (XEXP (this_op, 0)) == MINUS) + && CONSTANT_P (XEXP (XEXP (this_op, 0), 0)) + && CONSTANT_P (XEXP (XEXP (this_op, 0), 1))) + { + ops[i].op = XEXP (XEXP (this_op, 0), 0); ops[n_ops].op = XEXP (XEXP (this_op, 0), 1); - ops[n_ops].neg = this_neg; + ops[n_ops].neg + = (GET_CODE (XEXP (this_op, 0)) == MINUS) ^ this_neg; n_ops++; changed = 1; canonicalized = 1; @@ -4141,16 +4161,21 @@ simplify_plus_minus (enum rtx_code code, enum mach else tem = simplify_binary_operation (ncode, mode, lhs, rhs); - /* Reject "simplifications" that just wrap the two - arguments in a CONST. Failure to do so can result - in infinite recursion with simplify_binary_operation - when it calls us to simplify CONST operations. */ - if (tem - && ! (GET_CODE (tem) == CONST - && GET_CODE (XEXP (tem, 0)) == ncode - && XEXP (XEXP (tem, 0), 0) == lhs - && XEXP (XEXP (tem, 0), 1) == rhs)) + if (tem) { + /* Reject "simplifications" that just wrap the two + arguments in a CONST. Failure to do so can result + in infinite recursion with simplify_binary_operation + when it calls us to simplify CONST operations. + Also, if we find such a simplification, don't try + any more combinations with this rhs: We must have + something like symbol+offset, ie. one of the + trivial CONST expressions we handle later. */ + if (GET_CODE (tem) == CONST + && GET_CODE (XEXP (tem, 0)) == ncode + && XEXP (XEXP (tem, 0), 0) == lhs + && XEXP (XEXP (tem, 0), 1) == rhs) + break; lneg &= rneg; if (GET_CODE (tem) == NEG) tem = XEXP (tem, 0), lneg = !lneg; Index: gcc/var-tracking.c =================================================================== --- gcc/var-tracking.c (revision 214898) +++ gcc/var-tracking.c (working copy) @@ -8375,7 +8375,15 @@ vt_expand_var_loc_chain (variable var, bitmap regs *pendrecp = pending_recursion; if (!pendrecp || !pending_recursion) - var->var_part[0].cur_loc = result; + { + if (result) + { + rtx tem = simplify_rtx (result); + if (tem) + result = tem; + } + var->var_part[0].cur_loc = result; + } return result; }