diff mbox series

[v2,1/2] phi-opt: Fix for failing maybe_push_res_to_seq in factor_out_conditional_operation [PR 116409]

Message ID 20240820191417.3585143-1-quic_apinski@quicinc.com
State New
Headers show
Series [v2,1/2] phi-opt: Fix for failing maybe_push_res_to_seq in factor_out_conditional_operation [PR 116409] | expand

Commit Message

Andrew Pinski Aug. 20, 2024, 7:14 p.m. UTC 
The code was assuming that maybe_push_res_to_seq would not fail if the gimple_extract_op returned true.
But for some cases when the function is pure rather than const, then it can fail.
This change moves around the code to check the result of maybe_push_res_to_seq instead of assuming it will
always work.

Changes since v1:
* v2: Instead of directly testing non-pure builtin functions change to test if maybe_push_res_to_seq fails.

Bootstrapped and tested on x86_64-linux-gnu with no regressions.

	PR  tree-optimization/116409

gcc/ChangeLog:

	* tree-ssa-phiopt.cc (factor_out_conditional_operation): Move
	maybe_push_res_to_seq before creating the phi node and the debug dump.
	Return false if maybe_push_res_to_seq fails.

gcc/testsuite/ChangeLog:

	* gcc.dg/torture/pr116409-1.c: New test.
	* gcc.dg/torture/pr116409-2.c: New test.

Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
---
 gcc/testsuite/gcc.dg/torture/pr116409-1.c |  7 ++++++
 gcc/testsuite/gcc.dg/torture/pr116409-2.c |  7 ++++++
 gcc/tree-ssa-phiopt.cc                    | 30 +++++++++++++++--------
 3 files changed, 34 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr116409-1.c
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr116409-2.c

Comments

Jeff Law Aug. 20, 2024, 8:01 p.m. UTC | #1 
On 8/20/24 1:14 PM, Andrew Pinski wrote:
> The code was assuming that maybe_push_res_to_seq would not fail if the gimple_extract_op returned true.
> But for some cases when the function is pure rather than const, then it can fail.
> This change moves around the code to check the result of maybe_push_res_to_seq instead of assuming it will
> always work.
> 
> Changes since v1:
> * v2: Instead of directly testing non-pure builtin functions change to test if maybe_push_res_to_seq fails.
> 
> Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> 
> 	PR  tree-optimization/116409
> 
> gcc/ChangeLog:
> 
> 	* tree-ssa-phiopt.cc (factor_out_conditional_operation): Move
> 	maybe_push_res_to_seq before creating the phi node and the debug dump.
> 	Return false if maybe_push_res_to_seq fails.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.dg/torture/pr116409-1.c: New test.
> 	* gcc.dg/torture/pr116409-2.c: New test.
OK
jeff
diff mbox series

Patch

diff --git a/gcc/testsuite/gcc.dg/torture/pr116409-1.c b/gcc/testsuite/gcc.dg/torture/pr116409-1.c
new file mode 100644
index 00000000000..7bf8d49c9a0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr116409-1.c
@@ -0,0 +1,7 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-frounding-math -fno-math-errno" } */
+double f(int c, double a, double b) {
+  if (c)
+    return __builtin_sqrt(a);
+  return __builtin_sqrt(b);
+}
diff --git a/gcc/testsuite/gcc.dg/torture/pr116409-2.c b/gcc/testsuite/gcc.dg/torture/pr116409-2.c
new file mode 100644
index 00000000000..c27f11312d9
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr116409-2.c
@@ -0,0 +1,7 @@ 
+/* { dg-do compile } */
+
+int f (int t, char *a, char *b) {
+  if (t)
+    return __builtin_strlen (a);
+  return __builtin_strlen (b);
+}
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index 2d4aba5b087..95bac330c8f 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -54,6 +54,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "dbgcnt.h"
 #include "tree-ssa-propagate.h"
 #include "tree-ssa-dce.h"
+#include "calls.h"
 
 /* Return the singleton PHI in the SEQ of PHIs for edges E0 and E1. */
 
@@ -370,6 +371,25 @@  factor_out_conditional_operation (edge e0, edge e1, gphi *phi,
   /* Create a new PHI stmt.  */
   result = PHI_RESULT (phi);
   temp = make_ssa_name (TREE_TYPE (new_arg0), NULL);
+
+  gimple_match_op new_op = arg0_op;
+
+  /* Create the operation stmt if possible and insert it.  */
+  new_op.ops[0] = temp;
+  gimple_seq seq = NULL;
+  result = maybe_push_res_to_seq (&new_op, &seq, result);
+
+  /* If we can't create the new statement, release the temp name
+     and return back.  */
+  if (!result)
+    {
+      release_ssa_name (temp);
+      return NULL;
+    }
+
+  gsi = gsi_after_labels (gimple_bb (phi));
+  gsi_insert_seq_before (&gsi, seq, GSI_CONTINUE_LINKING);
+
   newphi = create_phi_node (temp, gimple_bb (phi));
 
   if (dump_file && (dump_flags & TDF_DETAILS))
@@ -398,16 +418,6 @@  factor_out_conditional_operation (edge e0, edge e1, gphi *phi,
   add_phi_arg (newphi, new_arg0, e0, locus);
   add_phi_arg (newphi, new_arg1, e1, locus);
 
-  gimple_match_op new_op = arg0_op;
-
-  /* Create the operation stmt and insert it.  */
-  new_op.ops[0] = temp;
-  gimple_seq seq = NULL;
-  result = maybe_push_res_to_seq (&new_op, &seq, result);
-  gcc_assert (result);
-  gsi = gsi_after_labels (gimple_bb (phi));
-  gsi_insert_seq_before (&gsi, seq, GSI_CONTINUE_LINKING);
-
   /* Remove the original PHI stmt.  */
   gsi = gsi_for_stmt (phi);
   gsi_remove (&gsi, true);