diff mbox series

Fix PR 111331: wrong code for `a > 28 ? MIN<a, 28> : 29`

Message ID 20230908123955.1196607-1-apinski@marvell.com
State Unresolved
Headers
Series Fix PR 111331: wrong code for `a > 28 ? MIN<a, 28> : 29` |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Andrew Pinski Sept. 8, 2023, 12:39 p.m. UTC 
  The problem here is after r6-7425-ga9fee7cdc3c62d0e51730,
the comparison to see if the transformation could be done was using the
wrong value. Instead of see if the inner was LE (for MIN and GE for MAX)
the outer value, it was comparing the inner to the value used in the comparison
which was wrong.
The match pattern copied the same logic mistake when they were added in
r14-1411-g17cca3c43e2f49 .

OK? Bootstrapped and tested on x86_64-linux-gnu.

gcc/ChangeLog:

	PR tree-optimization/111331
	* match.pd (`(a CMP CST1) ? max<a,CST2> : a`):
	Fix the LE/GE comparison to the correct value.
	* tree-ssa-phiopt.cc (minmax_replacement):
	Fix the LE/GE comparison for the
	`(a CMP CST1) ? max<a,CST2> : a` optimization.

gcc/testsuite/ChangeLog:

	PR tree-optimization/111331
	* gcc.c-torture/execute/pr111331-1.c: New test.
	* gcc.c-torture/execute/pr111331-2.c: New test.
	* gcc.c-torture/execute/pr111331-3.c: New test.
---
 gcc/match.pd                                  |  4 ++--
 .../gcc.c-torture/execute/pr111331-1.c        | 17 +++++++++++++++++
 .../gcc.c-torture/execute/pr111331-2.c        | 19 +++++++++++++++++++
 .../gcc.c-torture/execute/pr111331-3.c        | 15 +++++++++++++++
 gcc/tree-ssa-phiopt.cc                        |  8 ++++----
 5 files changed, 57 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr111331-1.c
 create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr111331-2.c
 create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr111331-3.c

Comments

Jeff Law Sept. 10, 2023, 3:39 p.m. UTC | #1 
On 9/8/23 06:39, Andrew Pinski via Gcc-patches wrote:
> The problem here is after r6-7425-ga9fee7cdc3c62d0e51730,
> the comparison to see if the transformation could be done was using the
> wrong value. Instead of see if the inner was LE (for MIN and GE for MAX)
> the outer value, it was comparing the inner to the value used in the comparison
> which was wrong.
> The match pattern copied the same logic mistake when they were added in
> r14-1411-g17cca3c43e2f49 .
> 
> OK? Bootstrapped and tested on x86_64-linux-gnu.
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/111331
> 	* match.pd (`(a CMP CST1) ? max<a,CST2> : a`):
> 	Fix the LE/GE comparison to the correct value.
> 	* tree-ssa-phiopt.cc (minmax_replacement):
> 	Fix the LE/GE comparison for the
> 	`(a CMP CST1) ? max<a,CST2> : a` optimization.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/111331
> 	* gcc.c-torture/execute/pr111331-1.c: New test.
> 	* gcc.c-torture/execute/pr111331-2.c: New test.
> 	* gcc.c-torture/execute/pr111331-3.c: New test.
OK
jeff
diff mbox series

Patch

diff --git a/gcc/match.pd b/gcc/match.pd
index 8c24dae71cd..c7b6db4b543 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -5438,11 +5438,11 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
     }
     (if ((cmp == LT_EXPR || cmp == LE_EXPR)
 	 && code == MIN_EXPR
-         && integer_nonzerop (fold_build2 (LE_EXPR, boolean_type_node, @3, @1)))
+         && integer_nonzerop (fold_build2 (LE_EXPR, boolean_type_node, @3, @4)))
      (min @2 @4)
      (if ((cmp == GT_EXPR || cmp == GE_EXPR)
 	  && code == MAX_EXPR
-          && integer_nonzerop (fold_build2 (GE_EXPR, boolean_type_node, @3, @1)))
+          && integer_nonzerop (fold_build2 (GE_EXPR, boolean_type_node, @3, @4)))
       (max @2 @4))))))
 
 #if GIMPLE
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr111331-1.c b/gcc/testsuite/gcc.c-torture/execute/pr111331-1.c
new file mode 100644
index 00000000000..4c7f4fdbaa9
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr111331-1.c
@@ -0,0 +1,17 @@ 
+int a;
+int b;
+int c(int d, int e, int f) {
+  if (d < e)
+    return e;
+  if (d > f)
+    return f;
+  return d;
+}
+int main() {
+  int g = -1;
+  a = c(b + 30, 29, g + 29);
+  volatile t = a;
+  if (t != 28)
+    __builtin_abort();
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr111331-2.c b/gcc/testsuite/gcc.c-torture/execute/pr111331-2.c
new file mode 100644
index 00000000000..5c677f2caa9
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr111331-2.c
@@ -0,0 +1,19 @@ 
+
+int a;
+int b;
+
+int main() {
+  int d = b+30;
+  {
+        int t;
+        if (d < 29)
+          t =  29;
+        else
+          t = (d > 28) ? 28 : d;
+    a = t;
+  }
+  volatile int t = a;
+  if (a != 28)
+    __builtin_abort();
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr111331-3.c b/gcc/testsuite/gcc.c-torture/execute/pr111331-3.c
new file mode 100644
index 00000000000..213d9bdd539
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr111331-3.c
@@ -0,0 +1,15 @@ 
+int a;
+int b;
+
+int main() {
+  int d = b+30;
+  {
+    int t;
+    t = d < 29 ? 29 : ((d > 28) ? 28 : d);
+    a = t;
+  }
+  volatile int t = a;
+  if (a != 28)
+    __builtin_abort();
+  return 0;
+}
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index 9993bbe5b76..9b44ca9758a 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -2073,7 +2073,7 @@  minmax_replacement (basic_block cond_bb, basic_block middle_bb, basic_block alt_
 
 	      /* We need BOUND <= LARGER.  */
 	      if (!integer_nonzerop (fold_build2 (LE_EXPR, boolean_type_node,
-						  bound, larger)))
+						  bound, arg_false)))
 		return false;
 	    }
 	  else if (operand_equal_for_phi_arg_p (arg_false, smaller)
@@ -2104,7 +2104,7 @@  minmax_replacement (basic_block cond_bb, basic_block middle_bb, basic_block alt_
 
 	      /* We need BOUND >= SMALLER.  */
 	      if (!integer_nonzerop (fold_build2 (GE_EXPR, boolean_type_node,
-						  bound, smaller)))
+						  bound, arg_false)))
 		return false;
 	    }
 	  else
@@ -2144,7 +2144,7 @@  minmax_replacement (basic_block cond_bb, basic_block middle_bb, basic_block alt_
 
 	      /* We need BOUND >= LARGER.  */
 	      if (!integer_nonzerop (fold_build2 (GE_EXPR, boolean_type_node,
-						  bound, larger)))
+						  bound, arg_true)))
 		return false;
 	    }
 	  else if (operand_equal_for_phi_arg_p (arg_true, smaller)
@@ -2171,7 +2171,7 @@  minmax_replacement (basic_block cond_bb, basic_block middle_bb, basic_block alt_
 
 	      /* We need BOUND <= SMALLER.  */
 	      if (!integer_nonzerop (fold_build2 (LE_EXPR, boolean_type_node,
-						  bound, smaller)))
+						  bound, arg_true)))
 		return false;
 	    }
 	  else