| Message ID | 20141020104656.GU4267@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Oct 20, 2014 at 09:16:57PM +1030, Alan Modra wrote: > PR debug/60655 > * simplify-rtx.c (simplify_plus_minus): Delete unused "input_ops". > Increase "ops" array size. Correct array size tests. Init > n_constants in loop. Break out of innermost loop when finding > a trivial CONST expression. LGTM, thanks. Jakub
On Mon, 20 Oct 2014, Alan Modra wrote: > You were correct to be suspicious that we weren't simplifying as we > should. After more time in the debugger than I care to admit, I found > the underlying cause. > > One of the var loc expressions is > (plus:SI (plus:SI (not:SI (debug_expr:SI D#9)) > (value/u:SI 58:4373 @0x18d3968/0x18ef230)) > (debug_expr:SI D#5)) > > which after substitution (in bb7) becomes > (plus:SI (plus:SI (not:SI (plus:SI (reg:SI 5 5 [orig:212 D.2333 ] [212]) > (const:SI (plus:SI (symbol_ref:SI ("*.LANCHOR0") [flags 0x182]) > (const_int -1 [0xffffffffffffffff]))))) > (reg:SI 10 10 [orig:223 ivtmp.33 ] [223])) > (plus:SI (reg:SI 5 5 [orig:212 D.2333 ] [212]) > (const:SI (plus:SI (symbol_ref:SI ("*.LANCHOR0") [flags 0x182]) > (const_int 323 [0x143]))))) > > The above has 8 ops by the time you turn ~x into -x - 1, and exceeds > the allowed number of elements in the simplify_plus_minus ops array. > Note that the ops array has 8 elements but the code only allows 7 to > be entered, a bug since the "spare" element isn't a sentinal or used > in any other way. > > This resulted in a partial simplification of the expression to > (plus:SI (plus:SI (reg:SI 10 10 [orig:223 ivtmp.33 ] [223]) > (symbol_ref:SI ("*.LANCHOR0") [flags 0x182])) > (const:SI (minus:SI (const_int 323 [0x143]) > (symbol_ref:SI ("*.LANCHOR0") [flags 0x182])))) > > I also noticed another small bug in simplify_plus_minus. n_constants > ought to be the number of constants in ops, not the number of times > we look at a constant. > > The "Handle CONST wrapped NOT, NEG and MINUS" in the previous patch > seems to no longer be necessary, so I took that out (didn't hit the > code in powerpc64-linux, powerpc-linux and x86_64-linux bootstrap and > regression tests). > > Bootstrapped and regression tested powerpc64-linux and x86_64-linux. For the record, I have now regression-tested your updated change too with my usual powerpc-gnu-linux multilibs: -mcpu=603e -mcpu=603e -msoft-float -mcpu=8540 -mfloat-gprs=single -mspe=yes -mabi=spe -mcpu=8548 -mfloat-gprs=double -mspe=yes -mabi=spe -mcpu=7400 -maltivec -mabi=altivec -mcpu=e6500 -maltivec -mabi=altivec -mcpu=e5500 -m64 -mcpu=e6500 -m64 -maltivec -mabi=altivec observing no issues. Thanks for the fix! Maciej
Index: gcc/simplify-rtx.c =================================================================== --- gcc/simplify-rtx.c (revision 216420) +++ gcc/simplify-rtx.c (working copy) @@ -3965,10 +3965,10 @@ simplify_plus_minus (enum rtx_code code, enum machine_mode mode, rtx op0, rtx op1) { - struct simplify_plus_minus_op_data ops[8]; + struct simplify_plus_minus_op_data ops[16]; rtx result, tem; - int n_ops = 2, input_ops = 2; - int changed, n_constants = 0, canonicalized = 0; + int n_ops = 2; + int changed, n_constants, canonicalized = 0; int i, j; memset (ops, 0, sizeof ops); @@ -3985,6 +3985,7 @@ do { changed = 0; + n_constants = 0; for (i = 0; i < n_ops; i++) { @@ -3996,7 +3997,7 @@ { case PLUS: case MINUS: - if (n_ops == 7) + if (n_ops == ARRAY_SIZE (ops)) return NULL_RTX; ops[n_ops].op = XEXP (this_op, 1); @@ -4004,7 +4005,6 @@ n_ops++; ops[i].op = XEXP (this_op, 0); - input_ops++; changed = 1; canonicalized |= this_neg; break; @@ -4017,7 +4017,7 @@ break; case CONST: - if (n_ops < 7 + if (n_ops != ARRAY_SIZE (ops) && GET_CODE (XEXP (this_op, 0)) == PLUS && CONSTANT_P (XEXP (XEXP (this_op, 0), 0)) && CONSTANT_P (XEXP (XEXP (this_op, 0), 1))) @@ -4033,7 +4033,7 @@ case NOT: /* ~a -> (-a - 1) */ - if (n_ops != 7) + if (n_ops != ARRAY_SIZE (ops)) { ops[n_ops].op = CONSTM1_RTX (mode); ops[n_ops++].neg = this_neg; @@ -4097,7 +4097,7 @@ /* Now simplify each pair of operands until nothing changes. */ do { - /* Insertion sort is good enough for an eight-element array. */ + /* Insertion sort is good enough for a small array. */ for (i = 1; i < n_ops; i++) { struct simplify_plus_minus_op_data save; @@ -4148,16 +4148,21 @@ 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;