public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT
@ 2021-07-04 18:37 apinski
  2021-07-04 18:37 ` [PATCH 2/5] Fix PR 101237: Remove element_type call when used with the functions from real apinski
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: apinski @ 2021-07-04 18:37 UTC (permalink / raw)
  To: gcc-patches; +Cc: Andrew Pinski

From: Andrew Pinski <apinski@marvell.com>

So the problem here is that replace_phi_edge_with_variable
will copy range information to a already (not newly) defined
ssa name.  This causes wrong code later on.
This patch fixes the problem by requiring there to be statements
that are to be placed before the conditional to be able to
copy the range info; this assumes the statements will define
the ssa name.

gcc/ChangeLog:

	PR tree-optimization/101256
	* dbgcnt.def (phiopt_edge_range): New counter.
	* tree-ssa-phiopt.c (replace_phi_edge_with_variable):
	Add optional sequence which will be added before the old
	conditional. Check sequence for non-null if we want to
	update the range.
	(two_value_replacement): Instead of inserting the sequence,
	update the call to replace_phi_edge_with_variable.
	(match_simplify_replacement): Likewise.
	(minmax_replacement): Likewise.
	(value_replacement): Create a sequence of statements
	which would have defined the ssa name.  Update call
	to replace_phi_edge_with_variable.

gcc/testsuite/ChangeLog:

	PR tree-optimization/101256
	* g++.dg/torture/pr101256.C: New test.
---
 gcc/dbgcnt.def                          |  1 +
 gcc/testsuite/g++.dg/torture/pr101256.C | 28 +++++++++++++
 gcc/tree-ssa-phiopt.c                   | 52 ++++++++++++++-----------
 3 files changed, 59 insertions(+), 22 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/torture/pr101256.C

diff --git a/gcc/dbgcnt.def b/gcc/dbgcnt.def
index 93e7b4fd30e..2345899ba68 100644
--- a/gcc/dbgcnt.def
+++ b/gcc/dbgcnt.def
@@ -183,6 +183,7 @@ DEBUG_COUNTER (lim)
 DEBUG_COUNTER (local_alloc_for_sched)
 DEBUG_COUNTER (match)
 DEBUG_COUNTER (merged_ipa_icf)
+DEBUG_COUNTER (phiopt_edge_range)
 DEBUG_COUNTER (postreload_cse)
 DEBUG_COUNTER (pre)
 DEBUG_COUNTER (pre_insn)
diff --git a/gcc/testsuite/g++.dg/torture/pr101256.C b/gcc/testsuite/g++.dg/torture/pr101256.C
new file mode 100644
index 00000000000..973a8b4caf3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr101256.C
@@ -0,0 +1,28 @@
+// { dg-do run }
+
+template<class T> 
+const T& max(const T& a, const T& b)
+{
+    return (a < b) ? b : a;
+}
+
+signed char var_5 = -128;
+unsigned int var_11 = 2144479212U;
+unsigned long long int arr [22];
+
+void
+__attribute__((noipa))
+test(signed char var_5, unsigned var_11) {
+  for (short i_61 = 0; i_61 < var_5 + 149; i_61 += 10000)
+    arr[i_61] = max((signed char)0, var_5) ? max((signed char)1, var_5) : var_11;
+}
+
+int main() {
+  for (int i_0 = 0; i_0 < 22; ++i_0) 
+      arr [i_0] = 11834725929543695741ULL;
+
+  test(var_5, var_11);
+  if (arr [0] != 2144479212ULL && arr [0] != 11834725929543695741ULL)
+    __builtin_abort ();
+  return 0;
+}
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index ab12e85569d..71f0019d877 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -50,6 +50,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimple-fold.h"
 #include "internal-fn.h"
 #include "gimple-range.h"
+#include "dbgcnt.h"
 
 static unsigned int tree_ssa_phiopt_worker (bool, bool, bool);
 static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
@@ -73,7 +74,8 @@ static bool cond_store_replacement (basic_block, basic_block, edge, edge,
 				    hash_set<tree> *);
 static bool cond_if_else_store_replacement (basic_block, basic_block, basic_block);
 static hash_set<tree> * get_non_trapping ();
-static void replace_phi_edge_with_variable (basic_block, edge, gphi *, tree);
+static void replace_phi_edge_with_variable (basic_block, edge, gphi *, tree,
+					    gimple_seq = NULL);
 static void hoist_adjacent_loads (basic_block, basic_block,
 				  basic_block, basic_block);
 static bool gate_hoist_loads (void);
@@ -382,18 +384,20 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
 
 /* Replace PHI node element whose edge is E in block BB with variable NEW.
    Remove the edge from COND_BLOCK which does not lead to BB (COND_BLOCK
-   is known to have two edges, one of which must reach BB).  */
+   is known to have two edges, one of which must reach BB).
+   Optionally insert stmts before the old condition.  */
 
 static void
 replace_phi_edge_with_variable (basic_block cond_block,
-				edge e, gphi *phi, tree new_tree)
+				edge e, gphi *phi, tree new_tree,
+				gimple_seq stmts)
 {
   basic_block bb = gimple_bb (phi);
   basic_block block_to_remove;
   gimple_stmt_iterator gsi;
   tree phi_result = PHI_RESULT (phi);
 
-  /* Duplicate range info if we're the only things setting the target PHI.
+  /* Duplicate range info if they are the only things setting the target PHI.
      This is needed as later on, the new_tree will be replacing
      The assignement of the PHI.
      For an example:
@@ -401,19 +405,23 @@ replace_phi_edge_with_variable (basic_block cond_block,
      _4 = min<a_1, 255>
      goto bb2
 
-     range<-INF,255>
+     # RANGE [-INF, 255]
      a_3 = PHI<_4(1)>
      bb3:
 
      use(a_3)
-     And _4 gets prograted into the use of a_3 and losing the range info.
-     This can't be done for more than 2 incoming edges as the progration
-     won't happen.  */
+     And _4 gets propagated into the use of a_3 and losing the range info.
+     This can't be done for more than 2 incoming edges as the propagation
+     won't happen.
+     This also cannot be done if the name is not defined by statements to
+     be placed before the conditional.  */
   if (TREE_CODE (new_tree) == SSA_NAME
       && EDGE_COUNT (gimple_bb (phi)->preds) == 2
       && INTEGRAL_TYPE_P (TREE_TYPE (phi_result))
       && !SSA_NAME_RANGE_INFO (new_tree)
-      && SSA_NAME_RANGE_INFO (phi_result))
+      && SSA_NAME_RANGE_INFO (phi_result)
+      && stmts != NULL
+      && dbg_cnt (phiopt_edge_range))
     duplicate_ssa_name_range_info (new_tree,
 				   SSA_NAME_RANGE_TYPE (phi_result),
 				   SSA_NAME_RANGE_INFO (phi_result));
@@ -443,6 +451,8 @@ replace_phi_edge_with_variable (basic_block cond_block,
 
   /* Eliminate the COND_EXPR at the end of COND_BLOCK.  */
   gsi = gsi_last_bb (cond_block);
+  if (stmts)
+    gsi_insert_seq_before (&gsi, stmts, GSI_SAME_STMT);
   gsi_remove (&gsi, true);
 
   statistics_counter_event (cfun, "Replace PHI with variable", 1);
@@ -802,10 +812,8 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
   else
     new_rhs = gimple_build (&stmts, MINUS_EXPR, type, arg, lhs);
   new_rhs = gimple_convert (&stmts, TREE_TYPE (arg0), new_rhs);
-  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
-  gsi_insert_seq_before (&gsi, stmts, GSI_SAME_STMT);
 
-  replace_phi_edge_with_variable (cond_bb, e1, phi, new_rhs);
+  replace_phi_edge_with_variable (cond_bb, e1, phi, new_rhs, stmts);
 
   /* Note that we optimized this PHI.  */
   return true;
@@ -920,10 +928,8 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
       gsi_move_before (&gsi1, &gsi);
       reset_flow_sensitive_info (gimple_assign_lhs (stmt_to_move));
     }
-  if (seq)
-    gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
 
-  replace_phi_edge_with_variable (cond_bb, e1, phi, result);
+  replace_phi_edge_with_variable (cond_bb, e1, phi, result, seq);
 
   /* Add Statistic here even though replace_phi_edge_with_variable already
      does it as we want to be able to count when match-simplify happens vs
@@ -1396,6 +1402,7 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
 		      && absorbing_element_p (code_def,
 					      cond_rhs, false, rhs2))))))
     {
+      gimple_seq stmts = NULL;
       gsi = gsi_for_stmt (cond);
       /* Moving ASSIGN might change VR of lhs, e.g. when moving u_6
 	 def-stmt in:
@@ -1417,11 +1424,15 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
 	  tree plhs = gimple_assign_lhs (prep_stmt[i]);
 	  reset_flow_sensitive_info (plhs);
 	  gsi_from = gsi_for_stmt (prep_stmt[i]);
-	  gsi_move_before (&gsi_from, &gsi);
+	  gsi_remove (&gsi_from, false);
+
+	  gimple_seq_add_stmt (&stmts, prep_stmt[i]);
 	}
       gsi_from = gsi_for_stmt (assign);
-      gsi_move_before (&gsi_from, &gsi);
-      replace_phi_edge_with_variable (cond_bb, e1, phi, lhs);
+      gsi_remove (&gsi_from, false);
+      gimple_seq_add_stmt (&stmts, assign);
+
+      replace_phi_edge_with_variable (cond_bb, e1, phi, lhs, stmts);
       return 2;
     }
 
@@ -1811,10 +1822,7 @@ minmax_replacement (basic_block cond_bb, basic_block middle_bb,
   tree phi_result = PHI_RESULT (phi);
   result = gimple_build (&stmts, minmax, TREE_TYPE (phi_result), arg0, arg1);
 
-  gsi = gsi_last_bb (cond_bb);
-  gsi_insert_seq_before (&gsi, stmts, GSI_NEW_STMT);
-
-  replace_phi_edge_with_variable (cond_bb, e1, phi, result);
+  replace_phi_edge_with_variable (cond_bb, e1, phi, result, stmts);
 
   return true;
 }
-- 
2.27.0


^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-07-05 22:54 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-04 18:37 [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT apinski
2021-07-04 18:37 ` [PATCH 2/5] Fix PR 101237: Remove element_type call when used with the functions from real apinski
2021-07-05 11:18   ` Richard Biener
2021-07-04 18:37 ` [PATCH 3/5] Allow match-and-simplified phiopt to run in early phiopt apinski
2021-07-05 11:26   ` Richard Biener
2021-07-04 18:37 ` [PATCH 4/5] Try inverted comparison for match_simplify in phiopt apinski
2021-07-05 11:18   ` Richard Biener
2021-07-04 18:37 ` [PATCH 5/5] Port most of the A CMP 0 ? A : -A to match apinski
2021-07-05 11:26   ` Richard Biener
2021-07-05 11:25 ` [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT Richard Biener
2021-07-05 22:54   ` Andrew Pinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).