Re: [PATCH] Combine related fail of gcc.target/powerpc/ti_math1.c

2015-06-22 Thread Alan Modra
On Mon, Jun 22, 2015 at 09:24:07AM +0200, Eric Botcazou wrote:
  * rtlanal.c (commutative_operand_precedence): Correct comments.
  * simplify-rtx.c (simplify_plus_minus_op_data_cmp): Delete forward
  declaration.  Return an int.  Distinguish REG,REG return from
  others.
  (struct simplify_plus_minus_op_data): Make local to function.
  (simplify_plus_minus): Rename canonicalized to not_canonical.
  Don't set not_canonical if merely sorting registers.  Avoid
  packing ops if nothing changes.  White space fixes.
 
 OK in principle, but...

Thanks for reviewing!

  Some notes: Renaming canonicalized to not_canonical better reflects
  its usage.  At the time the var is set, the expression hasn't been
  canonicalized.
 
 I'm quite skeptical, in particular given:

I'm a little surprised, but committed without the renaming.

-- 
Alan Modra
Australia Development Lab, IBM


Re: [PATCH] Combine related fail of gcc.target/powerpc/ti_math1.c

2015-06-22 Thread Eric Botcazou
 This patch fixes
 FAIL: gcc.target/powerpc/ti_math1.c scan-assembler-times adde 1
 a failure caused by combine simplifying this i2src
 
 (plus:DI (plus:DI (reg:DI 165 [ val+8 ])
 (reg:DI 169 [+8 ]))
 (reg:DI 76 ca))
 
 to this
 
 (plus:DI (plus:DI (reg:DI 76 ca)
 (reg:DI 165 [ val+8 ]))
 (reg:DI 169 [+8 ]))
 
 which no longer matches rs6000.md adddi3_carry_in_internal.  See
 https://gcc.gnu.org/ml/gcc/2015-05/msg00206.html for related
 discussion.  Bootstrapped and regression tested powerpc64le-linux,
 powerpc64-linux and x86_64-linux.  OK to apply mainline?
 
   * rtlanal.c (commutative_operand_precedence): Correct comments.
   * simplify-rtx.c (simplify_plus_minus_op_data_cmp): Delete forward
   declaration.  Return an int.  Distinguish REG,REG return from
   others.
   (struct simplify_plus_minus_op_data): Make local to function.
   (simplify_plus_minus): Rename canonicalized to not_canonical.
   Don't set not_canonical if merely sorting registers.  Avoid
   packing ops if nothing changes.  White space fixes.

OK in principle, but...

 Some notes: Renaming canonicalized to not_canonical better reflects
 its usage.  At the time the var is set, the expression hasn't been
 canonicalized.

I'm quite skeptical, in particular given:

+ /* Just swapping registers doesn't count as canonicalization.  */
+ if (cmp != 1)
+   not_canonical = 1;

and

+  /* If nothing changed, fail.  */
+  if (!not_canonical)
+return NULL_RTX;

Both are rather confusing now so the renaming isn't really a progress IMO.

-- 
Eric Botcazou


Re: [PATCH] Combine related fail of gcc.target/powerpc/ti_math1.c

2015-06-21 Thread Alan Modra
On Fri, May 22, 2015 at 04:04:19PM +0930, Alan Modra wrote:
https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02055.html

   * rtlanal.c (commutative_operand_precedence): Correct comments.
   * simplify-rtx.c (simplify_plus_minus_op_data_cmp): Delete forward
   declaration.  Return an int.  Distinguish REG,REG return from
   others.
   (struct simplify_plus_minus_op_data): Make local to function.
   (simplify_plus_minus): Rename canonicalized to not_canonical.
   Don't set not_canonical if merely sorting registers.  Avoid
   packing ops if nothing changes.  White space fixes.

Ping?

-- 
Alan Modra
Australia Development Lab, IBM


[PATCH] Combine related fail of gcc.target/powerpc/ti_math1.c

2015-05-22 Thread Alan Modra
This patch fixes
FAIL: gcc.target/powerpc/ti_math1.c scan-assembler-times adde 1
a failure caused by combine simplifying this i2src

(plus:DI (plus:DI (reg:DI 165 [ val+8 ])
(reg:DI 169 [+8 ]))
(reg:DI 76 ca))

to this

(plus:DI (plus:DI (reg:DI 76 ca)
(reg:DI 165 [ val+8 ]))
(reg:DI 169 [+8 ]))

which no longer matches rs6000.md adddi3_carry_in_internal.  See
https://gcc.gnu.org/ml/gcc/2015-05/msg00206.html for related
discussion.  Bootstrapped and regression tested powerpc64le-linux,
powerpc64-linux and x86_64-linux.  OK to apply mainline?

* rtlanal.c (commutative_operand_precedence): Correct comments.
* simplify-rtx.c (simplify_plus_minus_op_data_cmp): Delete forward
declaration.  Return an int.  Distinguish REG,REG return from
others.
(struct simplify_plus_minus_op_data): Make local to function.
(simplify_plus_minus): Rename canonicalized to not_canonical.
Don't set not_canonical if merely sorting registers.  Avoid
packing ops if nothing changes.  White space fixes.

Some notes: Renaming canonicalized to not_canonical better reflects
its usage.  At the time the var is set, the expression hasn't been
canonicalized.
diff -w shown to exclude the whitespace fixes.

Index: gcc/rtlanal.c
===
--- gcc/rtlanal.c   (revision 223463)
+++ gcc/rtlanal.c   (working copy)
@@ -3140,10 +3140,9 @@ regno_use_in (unsigned int regno, rtx x)
 }
 
 /* Return a value indicating whether OP, an operand of a commutative
-   operation, is preferred as the first or second operand.  The higher
-   the value, the stronger the preference for being the first operand.
-   We use negative values to indicate a preference for the first operand
-   and positive values for the second operand.  */
+   operation, is preferred as the first or second operand.  The more
+   positive the value, the stronger the preference for being the first
+   operand.  */
 
 int
 commutative_operand_precedence (rtx op)
@@ -3150,7 +3149,7 @@ commutative_operand_precedence (rtx op)
 {
   enum rtx_code code = GET_CODE (op);
 
-  /* Constants always come the second operand.  Prefer nice constants.  */
+  /* Constants always become the second operand.  Prefer nice constants.  */
   if (code == CONST_INT)
 return -8;
   if (code == CONST_WIDE_INT)
Index: gcc/simplify-rtx.c
===
--- gcc/simplify-rtx.c  (revision 223463)
+++ gcc/simplify-rtx.c  (working copy)
@@ -71,7 +71,6 @@ along with GCC; see the file COPYING3.  If not see
 
 static rtx neg_const_int (machine_mode, const_rtx);
 static bool plus_minus_operand_p (const_rtx);
-static bool simplify_plus_minus_op_data_cmp (rtx, rtx);
 static rtx simplify_plus_minus (enum rtx_code, machine_mode, rtx, rtx);
 static rtx simplify_immed_subreg (machine_mode, rtx, machine_mode,
  unsigned int);
@@ -4064,20 +4063,10 @@ simplify_const_binary_operation (enum rtx_code cod
 
 
 
-/* Simplify a PLUS or MINUS, at least one of whose operands may be another
-   PLUS or MINUS.
+/* Return a positive integer if X should sort after Y.  The value
+   returned is 1 if and only if X and Y are both regs.  */
 
-   Rather than test for specific case, we do this by a brute-force method
-   and do all possible simplifications until no more changes occur.  Then
-   we rebuild the operation.  */
-
-struct simplify_plus_minus_op_data
-{
-  rtx op;
-  short neg;
-};
-
-static bool
+static int
 simplify_plus_minus_op_data_cmp (rtx x, rtx y)
 {
   int result;
@@ -4085,23 +4074,36 @@ simplify_plus_minus_op_data_cmp (rtx x, rtx y)
   result = (commutative_operand_precedence (y)
- commutative_operand_precedence (x));
   if (result)
-return result  0;
+return result + result;
 
   /* Group together equal REGs to do more simplification.  */
   if (REG_P (x)  REG_P (y))
 return REGNO (x)  REGNO (y);
-  else
-return false;
+
+  return 0;
 }
 
+/* Simplify and canonicalize a PLUS or MINUS, at least one of whose
+   operands may be another PLUS or MINUS.
+
+   Rather than test for specific case, we do this by a brute-force method
+   and do all possible simplifications until no more changes occur.  Then
+   we rebuild the operation.
+
+   May return NULL_RTX when no changes were made.  */
+
 static rtx
 simplify_plus_minus (enum rtx_code code, machine_mode mode, rtx op0,
 rtx op1)
 {
-  struct simplify_plus_minus_op_data ops[16];
+  struct simplify_plus_minus_op_data
+  {
+rtx op;
+short neg;
+  } ops[16];
   rtx result, tem;
   int n_ops = 2;
-  int changed, n_constants, canonicalized = 0;
+  int changed, n_constants, not_canonical = 0;
   int i, j;
 
   memset (ops, 0, sizeof ops);
@@ -4139,7 +4141,18 @@ simplify_plus_minus (enum rtx_code code, machine_m
 
  ops[i].op = XEXP (this_op, 0);
  changed = 1;
-