[PATCH][PR debug/60655] Power/GCC: Reject cross-section symbol subtraction

[PATCH][PR debug/60655] Power/GCC: Reject cross-section symbol subtraction

[Maciej W. Rozycki]

Hi,

 Similarly to ARM, where this issue was seen originally, and likely many 
other targets, the Power ABI does not appear to have a relocation defined 
to support taking a difference of two symbols in different sections each. 
This is seen as a failure in gcc.c-torture/compile/pr60655-2.c:

Executing on host: powerpc-linux-gnu-gcc  -fno-diagnostics-show-caret -fdiagnostics-color=never   -O3 -g  -w -c  -o pr60655-2.o .../gcc/testsuite/gcc.c-torture/compile/pr60655-2.c    (timeout = 300)
/tmp/ccAfNLMj.s: Assembler messages:
/tmp/ccAfNLMj.s:932: Error: can't resolve `L0^A' {*ABS* section} - `.LANCHOR0' {.bss section}
/tmp/ccAfNLMj.s:932: Error: expression too complex
compiler exited with status 1
output is:
/tmp/ccAfNLMj.s: Assembler messages:
/tmp/ccAfNLMj.s:932: Error: can't resolve `L0^A' {*ABS* section} - `.LANCHOR0' {.bss section}
/tmp/ccAfNLMj.s:932: Error: expression too complex

FAIL: gcc.c-torture/compile/pr60655-2.c  -O3 -g  (test for excess errors)
Excess errors:
/tmp/ccAfNLMj.s:932: Error: can't resolve `L0^A' {*ABS* section} - `.LANCHOR0' {.bss section}
/tmp/ccAfNLMj.s:932: Error: expression too complex

Here's a port of the original ARM fix (commit 209269), that removes the 
failure for me.

 Regression-tested with the following 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

 OK for trunk and 4.9?

2014-09-02  Maciej W. Rozycki  <macro@codesourcery.com>

	PR debug/60655
	* config/rs6000/rs6000.c (rs6000_const_not_ok_for_debug_p):
	Reject MINUS with SYM_REFs in different sections.

  Maciej

gcc-rs6000-minus-not-ok-for-debug.diff
Index: gcc-fsf-trunk-quilt/gcc/config/rs6000/rs6000.c
===================================================================
--- gcc-fsf-trunk-quilt.orig/gcc/config/rs6000/rs6000.c	2014-08-26 20:30:10.348973028 +0100
+++ gcc-fsf-trunk-quilt/gcc/config/rs6000/rs6000.c	2014-09-01 17:09:23.748927487 +0100
@@ -6974,7 +6974,13 @@ rs6000_delegitimize_address (rtx orig_x)
 
 /* Return true if X shouldn't be emitted into the debug info.
    The linker doesn't like .toc section references from
-   .debug_* sections, so reject .toc section symbols.  */
+   .debug_* sections, so reject .toc section symbols.
+
+   Also as a temporary fix for PR60655 we reject certain MINUS
+   expressions.  Ideally we need to handle most of these cases in
+   the generic part but currently we reject minus (..) (sym_ref).
+   We try to ameliorate the case with minus (sym_ref1) (sym_ref2)
+   where they are in the same section.  */
 
 static bool
 rs6000_const_not_ok_for_debug_p (rtx x)
@@ -6988,6 +6994,35 @@ rs6000_const_not_ok_for_debug_p (rtx x)
 	return true;
     }
 
+  if (GET_CODE (x) == MINUS)
+    {
+      tree decl_op0 = NULL;
+      tree decl_op1 = NULL;
+
+      if (GET_CODE (XEXP (x, 1)) == SYMBOL_REF)
+       {
+	 decl_op1 = SYMBOL_REF_DECL (XEXP (x, 1));
+	 if (decl_op1
+	     && GET_CODE (XEXP (x, 0)) == SYMBOL_REF
+	     && (decl_op0 = SYMBOL_REF_DECL (XEXP (x, 0))))
+	   {
+	     if ((TREE_CODE (decl_op1) == VAR_DECL
+		  || TREE_CODE (decl_op1) == CONST_DECL)
+		 && (TREE_CODE (decl_op0) == VAR_DECL
+		     || TREE_CODE (decl_op0) == CONST_DECL))
+	       return (get_variable_section (decl_op1, false)
+		       != get_variable_section (decl_op0, false));
+
+	     if (TREE_CODE (decl_op1) == LABEL_DECL
+		 && TREE_CODE (decl_op0) == LABEL_DECL)
+	       return (DECL_CONTEXT (decl_op1)
+		       != DECL_CONTEXT (decl_op0));
+	   }
+
+	 return true;
+       }
+    }
+
   return false;
 }
 

Re: [PATCH][PR debug/60655] Power/GCC: Reject cross-section symbol subtraction

[Alan Modra]

On Tue, Sep 02, 2014 at 09:47:15AM -0400, David Edelsohn wrote:
> Alan,
> 
> Any feedback?

It is just papering over the real bug(s), of course, so I'd be
inclined to say this doesn't belong on trunk.

If you take a look at the assembly that is failing, you find gcc is
trying to output a location expression for a parameter of mp_compare.
After replacing the 335-.LANCHOR0 with 0:

objdump -sj.debug_loc
 0180                   000000f8 00000104
 0190 000f7a00 03000000 00220300 00000022
 01a0 9f

readelf -r
00000195  00000401 R_PPC_ADDR32           00000000   .bss + 0

readelf -wo
    00000188 000000f8 00000104 (DW_OP_breg10 (r10): 0; DW_OP_addr: 0; DW_OP_plus; DW_OP_addr: 0; DW_OP_plus; DW_OP_stack_value)

Putting this all together, and knowing that .LANCHOR0 == .bss+0, the
expresssion is really:

    00000188 000000f8 00000104 (DW_OP_breg10 (r10): 0; DW_OP_addr: .LANCHOR0; DW_OP_plus; DW_OP_addr: 335-.LANCHOR0; DW_OP_plus; DW_OP_stack_value)

ie. ((r10) + 0) + (.LANCHOR0) + (335-.LANCHOR0)

Clearly this could be simplified to (r10) + 335, so it might be that
var-tracking is being confused by section anchors.  However, it is
more than just a missing simplification because the location
expression doesn't make any sense at all..


In fact if you look at the corresponding location expression for
-fno-section-anchors code you get something quite screwy too.

0000018f 00000108 00000114 (DW_OP_breg10 (r10): 0; DW_OP_addr: 144; DW_OP_plus; DW_OP_stack_value

which is (r10) + &modulus.

If I have the interpretation of all the debug info correct, what this
is saying is that in the epilogue of upton_modmult, the value for the
"r2" parameter of mp_compare is found by adding the address of
"modulus" to the contents of r10.  The correct answer is simply that
the "r2" parameter of mp_compare is in r10.


> On Tue, Sep 2, 2014 at 6:28 AM, Maciej W. Rozycki
> <macro@codesourcery.com> wrote:
> > Hi,
> >
> >  Similarly to ARM, where this issue was seen originally, and likely many
> > other targets, the Power ABI does not appear to have a relocation defined
> > to support taking a difference of two symbols in different sections each.
> > This is seen as a failure in gcc.c-torture/compile/pr60655-2.c:

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH][PR debug/60655] Power/GCC: Reject cross-section symbol subtraction

[Alan Modra]

On Wed, Sep 03, 2014 at 03:01:17PM +0930, Alan Modra wrote:
> In fact if you look at the corresponding location expression for
> -fno-section-anchors code you get something quite screwy too.
> 
> 0000018f 00000108 00000114 (DW_OP_breg10 (r10): 0; DW_OP_addr: 144; DW_OP_plus; DW_OP_stack_value
> 
> which is (r10) + &modulus.

Fixed with this obvious patch.  Emitting part of a .debug_loc expression
is worse than no expression.  Bootstrapped and regression tested
x86_64-linux and committed revision 214899.

	PR debug/60655
	* dwarf2out.c (mem_loc_descriptor <PLUS>): Return NULL if addend
	can't be output.

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 214898)
+++ gcc/dwarf2out.c	(working copy)
@@ -12699,7 +12699,7 @@ mem_loc_descriptor (rtx rtl, enum machine_mode mod
 	      op1 = mem_loc_descriptor (XEXP (rtl, 1), mode, mem_mode,
 					VAR_INIT_STATUS_INITIALIZED);
 	      if (op1 == 0)
-		break;
+		return NULL;
 	      add_loc_descr (&mem_loc_result, op1);
 	      add_loc_descr (&mem_loc_result,
 			     new_loc_descr (DW_OP_plus, 0, 0));

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH][PR debug/60655] Power/GCC: Reject cross-section symbol subtraction

[Maciej W. Rozycki]

On Thu, 4 Sep 2014, Alan Modra wrote:

> > In fact if you look at the corresponding location expression for
> > -fno-section-anchors code you get something quite screwy too.
> > 
> > 0000018f 00000108 00000114 (DW_OP_breg10 (r10): 0; DW_OP_addr: 144; DW_OP_plus; DW_OP_stack_value
> > 
> > which is (r10) + &modulus.
> 
> Fixed with this obvious patch.  Emitting part of a .debug_loc expression
> is worse than no expression.  Bootstrapped and regression tested
> x86_64-linux and committed revision 214899.
> 
> 	PR debug/60655
> 	* dwarf2out.c (mem_loc_descriptor <PLUS>): Return NULL if addend
> 	can't be output.

 Thanks for your analysis and the fix, unfortunately the test case still 
fails here, so it must be something else yet. :(

  Maciej

Re: [PATCH][PR debug/60655] Power/GCC: Reject cross-section symbol subtraction

[Alan Modra]

On Thu, Sep 04, 2014 at 10:05:38PM +0100, Maciej W. Rozycki wrote:
> On Thu, 4 Sep 2014, Alan Modra wrote:
> 
> > > In fact if you look at the corresponding location expression for
> > > -fno-section-anchors code you get something quite screwy too.
> > > 
> > > 0000018f 00000108 00000114 (DW_OP_breg10 (r10): 0; DW_OP_addr: 144; DW_OP_plus; DW_OP_stack_value
> > > 
> > > which is (r10) + &modulus.
> > 
> > Fixed with this obvious patch.  Emitting part of a .debug_loc expression
> > is worse than no expression.  Bootstrapped and regression tested
> > x86_64-linux and committed revision 214899.
> > 
> > 	PR debug/60655
> > 	* dwarf2out.c (mem_loc_descriptor <PLUS>): Return NULL if addend
> > 	can't be output.
> 
>  Thanks for your analysis and the fix, unfortunately the test case still 
> fails here, so it must be something else yet. :(

Yes, my patch wasn't supposed to fix the testcase, just ensure that we
don't get partial (and thus wrong) expressions emitted when
const_ok_for_output rejects part of a location description.  Jakub's
2014-04-04 patch rejected NOT, which is what we happen to hit for
-fno-section-anchors.

Your patch rejects the problematic MINUS case seen with
-fsection-anchors, and prior to my mem_loc_descriptor fix, left us
with bogus debug info.  That bogus debug info was my main objection to
your patch going onto trunk.  Since that is now fixed and rejecting
the MINUS is no worse than rejecting NOT, I withdraw that objection.

Of course it would be better to repair the damage done to debug info
rather than rejecting it outright..

-- 
Alan Modra
Australia Development Lab, IBM

PR debug/60655, debug loc expressions

[Alan Modra]

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);
+	  if (tem)
+	    result = tem;
+	}
+      var->var_part[0].cur_loc = result;
+    }
 
   return result;
 }

-- 
Alan Modra
Australia Development Lab, IBM

Re: PR debug/60655, debug loc expressions

[Richard Biener]

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

Re: PR debug/60655, debug loc expressions

[Jakub Jelinek]

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

Re: PR debug/60655, debug loc expressions

[Richard Biener]

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

Re: PR debug/60655, debug loc expressions

[Alan Modra]

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.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PR debug/60655, debug loc expressions

[Maciej W. Rozycki]

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

Re: PR debug/60655, debug loc expressions

[Alan Modra]

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.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PR debug/60655, debug loc expressions

[Maciej W. Rozycki]

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

Re: PR debug/60655, debug loc expressions

[Ramana Radhakrishnan]

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

Re: PR debug/60655, debug loc expressions

[Maciej W. Rozycki]

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

Re: PR debug/60655, debug loc expressions

[Alan Modra]

Ping?
https://gcc.gnu.org/ml/gcc-patches/2014-09/msg00704.html

-- 
Alan Modra
Australia Development Lab, IBM

Re: PR debug/60655, debug loc expressions

[Jakub Jelinek]

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

Re: PR debug/60655, debug loc expressions

[Alan Modra]

On Thu, Oct 16, 2014 at 09:07:58AM +0200, Jakub Jelinek wrote:
> So, please find the spot where we forget to simplify stuff, and put the
> simplification there.

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.
OK to apply?

	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.

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;

-- 
Alan Modra
Australia Development Lab, IBM

Re: PR debug/60655, debug loc expressions

[Jakub Jelinek]

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

Fix 63615 - FAIL: gcc.target/i386/addr-sel-1.c

[Alan Modra]

PR 63615 was caused by r21642 fixing the accounting of "n_constants"
in simplify_plus_minus.  Previously, any expression handled by
simplify_plus_minus that expanded to more than two elements and
contained at least one constant would have resulted in "n_constants"
being larger than one, even if it had only one constant.  This had the
effect of setting "canonicalized" for such expressions.

The missed optimisation had these operands to simplify_plus_minus:
(gdb) p debug_rtx(op0)
(plus:SI (reg:SI 0 ax [96])
    (const_int 1 [0x1]))
$1 = void
(gdb) p debug_rtx(op1)
(symbol_ref:SI ("a") <var_decl 0x7ffff6c2f900 a>)
$2 = void

resulting in the ops array being populated as
(gdb) p n_ops
$3 = 3
(gdb) p ops[0]@3
$4 = {{op = 0x7ffff6d4b360, neg = 0}, {op = 0x7ffff6d483a8, neg = 0}, {op = 0x7ffff6c29490, neg = 0}}
(gdb) p debug_rtx(ops[0].op)
(reg:SI 0 ax [96])
$5 = void
(gdb) p debug_rtx(ops[1].op)
(symbol_ref:SI ("a") <var_decl 0x7ffff6c2f900 a>)
$6 = void
(gdb) p debug_rtx(ops[2].op)
(const_int 1 [0x1])
$7 = void

Of note here is that the operands have been reordered from their
original positions.  What was ax + 1 + sym is now ax + sym + 1, and it
happens that this ordering is correct in the sense that
simplify_plus_minus_op_data_cmp sorting of the ops array produces no
changes.  Now any change made during sorting sets "canonicalized", so
I figure reordering while decomposing operands ought to set
"canonicalized" too.  Indeed, the reordering seen above has
canonicalized the expression.  (Of course the reordering during
decomposition might be exactly cancelled by later sorting, but it
hardly seems worth fixing that, and other cases where we might return
the input expression unchanged..)

I'm still running bootstrap and regression tests on x86_64-linux,
this time with both -m64 and -m32 regression tests.
OK to apply assuming no regressions?

	PR rtl-optimization/63615
	* simplify-rtx.c (simplify_plus_minus): Set "canonicalized" on
	decomposing PLUS or MINUS if operands are not placed adjacent
	in the "ops" array.

Index: gcc/simplify-rtx.c
===================================================================
--- gcc/simplify-rtx.c	(revision 216573)
+++ gcc/simplify-rtx.c	(working copy)
@@ -4006,7 +4006,7 @@
 
 	      ops[i].op = XEXP (this_op, 0);
 	      changed = 1;
-	      canonicalized |= this_neg;
+	      canonicalized |= this_neg || i != n_ops - 2;
 	      break;
 
 	    case NEG:

-- 
Alan Modra
Australia Development Lab, IBM

Re: Fix 63615 - FAIL: gcc.target/i386/addr-sel-1.c

[Jeff Law]

On 10/23/14 00:56, Alan Modra wrote:
> PR 63615 was caused by r21642 fixing the accounting of "n_constants"
> in simplify_plus_minus.  Previously, any expression handled by
> simplify_plus_minus that expanded to more than two elements and
> contained at least one constant would have resulted in "n_constants"
> being larger than one, even if it had only one constant.  This had the
> effect of setting "canonicalized" for such expressions.
>
> The missed optimisation had these operands to simplify_plus_minus:
> (gdb) p debug_rtx(op0)
> (plus:SI (reg:SI 0 ax [96])
>      (const_int 1 [0x1]))
> $1 = void
> (gdb) p debug_rtx(op1)
> (symbol_ref:SI ("a") <var_decl 0x7ffff6c2f900 a>)
> $2 = void
>
> resulting in the ops array being populated as
> (gdb) p n_ops
> $3 = 3
> (gdb) p ops[0]@3
> $4 = {{op = 0x7ffff6d4b360, neg = 0}, {op = 0x7ffff6d483a8, neg = 0}, {op = 0x7ffff6c29490, neg = 0}}
> (gdb) p debug_rtx(ops[0].op)
> (reg:SI 0 ax [96])
> $5 = void
> (gdb) p debug_rtx(ops[1].op)
> (symbol_ref:SI ("a") <var_decl 0x7ffff6c2f900 a>)
> $6 = void
> (gdb) p debug_rtx(ops[2].op)
> (const_int 1 [0x1])
> $7 = void
>
> Of note here is that the operands have been reordered from their
> original positions.  What was ax + 1 + sym is now ax + sym + 1, and it
> happens that this ordering is correct in the sense that
> simplify_plus_minus_op_data_cmp sorting of the ops array produces no
> changes.  Now any change made during sorting sets "canonicalized", so
> I figure reordering while decomposing operands ought to set
> "canonicalized" too.  Indeed, the reordering seen above has
> canonicalized the expression.  (Of course the reordering during
> decomposition might be exactly cancelled by later sorting, but it
> hardly seems worth fixing that, and other cases where we might return
> the input expression unchanged..)
>
> I'm still running bootstrap and regression tests on x86_64-linux,
> this time with both -m64 and -m32 regression tests.
> OK to apply assuming no regressions?
>
> 	PR rtl-optimization/63615
> 	* simplify-rtx.c (simplify_plus_minus): Set "canonicalized" on
> 	decomposing PLUS or MINUS if operands are not placed adjacent
> 	in the "ops" array.
OK assuming no regressions.

jeff

Re: PR debug/60655, debug loc expressions

[Maciej W. Rozycki]

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