diff mbox

[PR66726] Fixe regression caused by Factor conversion out of COND_EXPR

Message ID 569DB2F9.30506@linaro.org
State New
Headers show

Commit Message

Kugan Vivekanandarajah Jan. 19, 2016, 3:52 a.m. UTC
Hi,

This is an updated version of
https://gcc.gnu.org/ml/gcc-patches/2015-07/msg02196.html.

Patch to fix PR66726 missed optimization, factor conversion out of
COND_EXPR caused a regression for targets with branch cost greater than
i.e., testcase gcc.dg/pr46309.c failed for these targets. I posted a
patch for this which had some issues. Please find an updated version of
this patch that now passes regression.

This patch makes optimize_range_tests understand the factored out
COND_EXPR. i.e., Updated the final_range_test_p to look for the new
pattern. Changed the maybe_optimize_range_tests (which does the inter
basic block range test optimization) accordingly.

Bootstrapped and regression tested on x86_64-none-linux-gnu with no new
regressions. And also regression tested on arm-none-linux-gnu and
aarch64-none-linux-gnu with no new regressions.
Is this Ok for trunk?

Thanks,
Kugan


gcc/ChangeLog:

2016-01-19  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR middle-end/66726
	* tree-ssa-reassoc.c (optimize_range_tests): Handle tcc_compare stmt
	whose result is used in PHI.
	(maybe_optimize_range_tests): Likewise.
	(final_range_test_p): Lokweise.

Comments

Jeff Law Feb. 8, 2016, 4:49 p.m. UTC | #1
On 01/18/2016 08:52 PM, Kugan wrote:
> Hi,
>
> This is an updated version of
> https://gcc.gnu.org/ml/gcc-patches/2015-07/msg02196.html.
>
> Patch to fix PR66726 missed optimization, factor conversion out of
> COND_EXPR caused a regression for targets with branch cost greater than
> i.e., testcase gcc.dg/pr46309.c failed for these targets. I posted a
> patch for this which had some issues. Please find an updated version of
> this patch that now passes regression.
>
> This patch makes optimize_range_tests understand the factored out
> COND_EXPR. i.e., Updated the final_range_test_p to look for the new
> pattern. Changed the maybe_optimize_range_tests (which does the inter
> basic block range test optimization) accordingly.
>
> Bootstrapped and regression tested on x86_64-none-linux-gnu with no new
> regressions. And also regression tested on arm-none-linux-gnu and
> aarch64-none-linux-gnu with no new regressions.
> Is this Ok for trunk?
>
> Thanks,
> Kugan
>
>
> gcc/ChangeLog:
>
> 2016-01-19  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
> 	PR middle-end/66726
> 	* tree-ssa-reassoc.c (optimize_range_tests): Handle tcc_compare stmt
> 	whose result is used in PHI.
> 	(maybe_optimize_range_tests): Likewise.
> 	(final_range_test_p): Lokweise.
>
s/Lokeweise/Likewise/ in the ChangeLog entry.

Otherwise this looks OK for the trunk.  It really hasn't changed much 
since the version from July.  And while the PR is not marked as such, 
this is a code quality regression fix for targets with a BRANCH_COST > 1.

Thanks for your patience and not letting this get lost.

Jeff
Markus Trippelsdorf Feb. 12, 2016, 6:18 a.m. UTC | #2
On 2016.02.08 at 09:49 -0700, Jeff Law wrote:
> On 01/18/2016 08:52 PM, Kugan wrote:
> >
> >2016-01-19  Kugan Vivekanandarajah  <kuganv@linaro.org>
> >
> >	PR middle-end/66726
> >	* tree-ssa-reassoc.c (optimize_range_tests): Handle tcc_compare stmt
> >	whose result is used in PHI.
> >	(maybe_optimize_range_tests): Likewise.
> >	(final_range_test_p): Lokweise.
> >
> Otherwise this looks OK for the trunk.  It really hasn't changed much since
> the version from July.  And while the PR is not marked as such, this is a
> code quality regression fix for targets with a BRANCH_COST > 1.

This causes LTO/PGO bootstrap on ppc64le to ICE all over the place:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69781
Kugan Vivekanandarajah Feb. 12, 2016, 6:27 a.m. UTC | #3
On 12/02/16 17:18, Markus Trippelsdorf wrote:
> On 2016.02.08 at 09:49 -0700, Jeff Law wrote:
>> On 01/18/2016 08:52 PM, Kugan wrote:
>>>
>>> 2016-01-19  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>
>>> 	PR middle-end/66726
>>> 	* tree-ssa-reassoc.c (optimize_range_tests): Handle tcc_compare stmt
>>> 	whose result is used in PHI.
>>> 	(maybe_optimize_range_tests): Likewise.
>>> 	(final_range_test_p): Lokweise.
>>>
>> Otherwise this looks OK for the trunk.  It really hasn't changed much since
>> the version from July.  And while the PR is not marked as such, this is a
>> code quality regression fix for targets with a BRANCH_COST > 1.
>
> This causes LTO/PGO bootstrap on ppc64le to ICE all over the place:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69781
>

Sorry for the breakage. I will revert the patch while I investigate this.

Thanks,
Kugan
Eric Botcazou Feb. 12, 2016, 8:39 a.m. UTC | #4
> This causes LTO/PGO bootstrap on ppc64le to ICE all over the place:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69781

And a comparison failure with -enable-checking=yes,rtl on x86-64/Linux:

Comparing stages 2 and 3
warning: gcc/cc1plus-checksum.o differs
warning: gcc/cc1-checksum.o differs
Bootstrap comparison failure!
gcc/cp/typeck2.o differs
gcc/cp/decl.o differs
gcc/cp/mangle.o differs
gcc/sched-deps.o differs
gcc/cselib.o differs
gcc/cfgexpand.o differs
gcc/ipa-icf-gimple.o differs
gcc/gcse.o differs
gcc/tree-ssa-alias.o differs
gcc/postreload-gcse.o differs
gcc/reginfo.o differs
gcc/sel-sched.o differs
gcc/build/genmatch.o differs
gcc/build/genrecog.o differs
gcc/gimplify.o differs
gcc/tree-inline.o differs
gcc/optabs.o differs
gcc/emit-rtl.o differs
gcc/dwarf2out.o differs
gcc/tree-cfg.o differs
gcc/tree-eh.o differs
gcc/tree-predcom.o differs
gcc/tree-chrec.o differs
gcc/opts-common.o differs
gcc/tree-ssa-phiopt.o differs
gcc/asan.o differs
gcc/var-tracking.o differs
gcc/ipa-prop.o differs
gcc/diagnostic-show-locus.o differs
gcc/predict.o differs
gcc/tree-vectorizer.o differs
gcc/auto-profile.o differs
gcc/libgcov-driver-tool.o differs
gcc/tree-chkp-opt.o differs
gcc/gcc.o differs
gcc/store-motion.o differs
gcc/sched-rgn.o differs
gcc/haifa-sched.o differs
gcc/ada/rtsfind.o differs
gcc/ada/ali.o differs
gcc/ada/adaint.o differs
gcc/ada/fname-uf.o differs
gcc/ada/set_targ.o differs
gcc/ipa-icf.o differs
gcc/builtins.o differs
gcc/fold-const.o differs
gcc/reload.o differs
gcc/tree-ssa-loop-ivopts.o differs
gcc/tree-sra.o differs
gcc/tree-dfa.o differs
gcc/gimple-ssa-strength-reduction.o differs
gcc/tree-ssa-address.o differs
gcc/gimple-ssa-backprop.o differs
gcc/postreload.o differs
gcc/wide-int.o differs
gcc/tree-switch-conversion.o differs
gcc/real.o differs
gcc/tree-ssa-loop-niter.o differs
gcc/gimple-match.o differs
gcc/simplify-rtx.o differs
gcc/tree-vrp.o differs
gcc/tree-if-conv.o differs
gcc/coverage.o differs
gcc/gimple-fold.o differs
gcc/function.o differs
gcc/i386.o differs
gcc/tree-affine.o differs
gcc/tree-vect-loop.o differs
gcc/tree-ssa-structalias.o differs
gcc/c/c-parser.o differs
gcc/cfgrtl.o differs
gcc/tree-ssa-loop-prefetch.o differs
gcc/generic-match.o differs
gcc/rtl-chkp.o differs
gcc/tree-ssa-coalesce.o differs
gcc/mcf.o differs
gcc/optabs-libfuncs.o differs
libbacktrace/dwarf.o differs
libbacktrace/.libs/dwarf.o differs
libcpp/lex.o differs
Makefile:21544: recipe for target 'compare' failed
H.J. Lu Feb. 12, 2016, 2:43 p.m. UTC | #5
On Thu, Feb 11, 2016 at 10:18 PM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2016.02.08 at 09:49 -0700, Jeff Law wrote:
>> On 01/18/2016 08:52 PM, Kugan wrote:
>> >
>> >2016-01-19  Kugan Vivekanandarajah  <kuganv@linaro.org>
>> >
>> >     PR middle-end/66726
>> >     * tree-ssa-reassoc.c (optimize_range_tests): Handle tcc_compare stmt
>> >     whose result is used in PHI.
>> >     (maybe_optimize_range_tests): Likewise.
>> >     (final_range_test_p): Lokweise.
>> >
>> Otherwise this looks OK for the trunk.  It really hasn't changed much since
>> the version from July.  And while the PR is not marked as such, this is a
>> code quality regression fix for targets with a BRANCH_COST > 1.
>
> This causes LTO/PGO bootstrap on ppc64le to ICE all over the place:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69781
>

On ia32, it also caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69786

and

FAIL: libgomp.oacc-fortran/collapse-4.f90 -DACC_DEVICE_TYPE_host=1
-DACC_MEM_SHARED=1  -O1  execution test
FAIL: libgomp.oacc-fortran/collapse-5.f90 -DACC_DEVICE_TYPE_host=1
-DACC_MEM_SHARED=1  -O1  execution test

which were compiled into infinite loop.
diff mbox

Patch

diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index e53cc56..d0a5cee 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -2687,18 +2687,33 @@  optimize_range_tests (enum tree_code opcode,
    # _345 = PHI <_123(N), 1(...), 1(...)>
    where _234 has bool type, _123 has single use and
    bb N has a single successor M.  This is commonly used in
+   the last block of a range test.
+
+   Also Return true if STMT is tcc_compare like:
+   <bb N>:
+   ...
+   _234 = a_2(D) == 2;
+
+   <bb M>:
+   # _345 = PHI <_234(N), 1(...), 1(...)>
+   _346 = (int) _345;
+   where _234 has booltype, single use and
+   bb N has a single successor M.  This is commonly used in
    the last block of a range test.  */
 
 static bool
 final_range_test_p (gimple *stmt)
 {
-  basic_block bb, rhs_bb;
+  basic_block bb, rhs_bb, lhs_bb;
   edge e;
   tree lhs, rhs;
   use_operand_p use_p;
   gimple *use_stmt;
 
-  if (!gimple_assign_cast_p (stmt))
+  if (!gimple_assign_cast_p (stmt)
+      && (!is_gimple_assign (stmt)
+	  || (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
+	      != tcc_comparison)))
     return false;
   bb = gimple_bb (stmt);
   if (!single_succ_p (bb))
@@ -2709,11 +2724,16 @@  final_range_test_p (gimple *stmt)
 
   lhs = gimple_assign_lhs (stmt);
   rhs = gimple_assign_rhs1 (stmt);
-  if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
-      || TREE_CODE (rhs) != SSA_NAME
-      || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
+  if (gimple_assign_cast_p (stmt)
+      && (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
+	  || TREE_CODE (rhs) != SSA_NAME
+	  || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE))
     return false;
 
+  if (!gimple_assign_cast_p (stmt)
+      && (TREE_CODE (TREE_TYPE (lhs)) != BOOLEAN_TYPE))
+      return false;
+
   /* Test whether lhs is consumed only by a PHI in the only successor bb.  */
   if (!single_imm_use (lhs, &use_p, &use_stmt))
     return false;
@@ -2723,10 +2743,20 @@  final_range_test_p (gimple *stmt)
     return false;
 
   /* And that the rhs is defined in the same loop.  */
-  rhs_bb = gimple_bb (SSA_NAME_DEF_STMT (rhs));
-  if (rhs_bb == NULL
-      || !flow_bb_inside_loop_p (loop_containing_stmt (stmt), rhs_bb))
-    return false;
+  if (gimple_assign_cast_p (stmt))
+    {
+      if (TREE_CODE (rhs) != SSA_NAME
+	  || !(rhs_bb = gimple_bb (SSA_NAME_DEF_STMT (rhs)))
+	  || !flow_bb_inside_loop_p (loop_containing_stmt (stmt), rhs_bb))
+	return false;
+    }
+  else
+    {
+      if (TREE_CODE (lhs) != SSA_NAME
+	  || !(lhs_bb = gimple_bb (SSA_NAME_DEF_STMT (lhs)))
+	  || !flow_bb_inside_loop_p (loop_containing_stmt (stmt), lhs_bb))
+	return false;
+    }
 
   return true;
 }
@@ -3119,6 +3149,8 @@  maybe_optimize_range_tests (gimple *stmt)
 
 	  /* stmt is
 	     _123 = (int) _234;
+	     OR
+	     _234 = a_2(D) == 2;
 
 	     followed by:
 	     <bb M>:
@@ -3148,6 +3180,8 @@  maybe_optimize_range_tests (gimple *stmt)
 	     of the bitwise or resp. and, recursively.  */
 	  if (!get_ops (rhs, code, &ops,
 			loop_containing_stmt (stmt))
+	      && (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
+		  != tcc_comparison)
 	      && has_single_use (rhs))
 	    {
 	      /* Otherwise, push the _234 range test itself.  */
@@ -3160,6 +3194,23 @@  maybe_optimize_range_tests (gimple *stmt)
 	      ops.safe_push (oe);
 	      bb_ent.last_idx++;
 	    }
+	  else if (!get_ops (lhs, code, &ops,
+			     loop_containing_stmt (stmt))
+		   && TREE_CODE (lhs) == SSA_NAME
+		   && INTEGRAL_TYPE_P (TREE_TYPE (lhs))
+		   && is_gimple_assign (stmt)
+		   && (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
+		       == tcc_comparison)
+		   && has_single_use (lhs))
+	    {
+	      operand_entry *oe = operand_entry_pool.allocate ();
+	      oe->op = lhs;
+	      oe->rank = code;
+	      oe->id = 0;
+	      oe->count = 1;
+	      ops.safe_push (oe);
+	      bb_ent.last_idx++;
+	    }
 	  else
 	    bb_ent.last_idx = ops.length ();
 	  bb_ent.op = rhs;
@@ -3243,26 +3294,60 @@  maybe_optimize_range_tests (gimple *stmt)
 		{
 		  imm_use_iterator iter;
 		  use_operand_p use_p;
-		  gimple *use_stmt, *cast_stmt = NULL;
+		  gimple *use_stmt, *cast_or_tcc_cmp_stmt = NULL;
 
 		  FOR_EACH_IMM_USE_STMT (use_stmt, iter, bbinfo[idx].op)
-		    if (is_gimple_debug (use_stmt))
+		    if (is_gimple_debug (use_stmt)
+			|| (TREE_CODE (new_op) == SSA_NAME
+			    && !reassoc_stmt_dominates_stmt_p
+			    (SSA_NAME_DEF_STMT (new_op), use_stmt)))
 		      continue;
-		    else if (gimple_code (use_stmt) == GIMPLE_COND
-			     || gimple_code (use_stmt) == GIMPLE_PHI)
-		      FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
-			SET_USE (use_p, new_op);
+		    else if (gimple_code (use_stmt) == GIMPLE_PHI)
+			FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
+			  SET_USE (use_p, new_op);
+		    else if (gimple_code (use_stmt) == GIMPLE_COND)
+		      {
+			tree new_type, new_lhs;
+			gassign *g;
+			gcond *cond_stmt = as_a <gcond *> (use_stmt);
+			new_type = TREE_TYPE (gimple_cond_lhs (cond_stmt));
+			if (!types_compatible_p (new_type, TREE_TYPE (new_op)))
+			  {
+			    new_lhs = make_ssa_name (new_type);
+			    if (is_gimple_min_invariant (new_op))
+			      {
+				new_op = fold_convert (new_type, new_op);
+				g = gimple_build_assign (new_lhs, new_op);
+			      }
+			    else
+			      g = gimple_build_assign (new_lhs,
+						       CONVERT_EXPR, new_op);
+			    gimple_stmt_iterator gsi = gsi_for_stmt (use_stmt);
+			    gimple_set_uid (g, gimple_uid (use_stmt));
+			    gimple_set_visited (g, true);
+			    gsi_insert_before (&gsi, g, GSI_SAME_STMT);
+			  }
+			else
+			  new_lhs = new_op;
+			FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
+			  SET_USE (use_p, new_lhs);
+		      }
+		    else if ((is_gimple_assign (use_stmt)
+			      && (TREE_CODE_CLASS (gimple_assign_rhs_code (use_stmt)) == tcc_comparison)))
+		      {
+			cast_or_tcc_cmp_stmt = use_stmt;
+		      }
 		    else if (gimple_assign_cast_p (use_stmt))
-		      cast_stmt = use_stmt;
-		    else
-		      gcc_unreachable ();
-		  if (cast_stmt)
+			cast_or_tcc_cmp_stmt = use_stmt;
+
+		  if (cast_or_tcc_cmp_stmt)
 		    {
 		      gcc_assert (bb == last_bb);
-		      tree lhs = gimple_assign_lhs (cast_stmt);
+		      tree lhs = gimple_assign_lhs (cast_or_tcc_cmp_stmt);
 		      tree new_lhs = make_ssa_name (TREE_TYPE (lhs));
 		      enum tree_code rhs_code
-			= gimple_assign_rhs_code (cast_stmt);
+			= gimple_assign_cast_p (cast_or_tcc_cmp_stmt) ?
+			gimple_assign_rhs_code (cast_or_tcc_cmp_stmt) : CONVERT_EXPR;
 		      gassign *g;
 		      if (is_gimple_min_invariant (new_op))
 			{
@@ -3271,13 +3356,14 @@  maybe_optimize_range_tests (gimple *stmt)
 			}
 		      else
 			g = gimple_build_assign (new_lhs, rhs_code, new_op);
-		      gimple_stmt_iterator gsi = gsi_for_stmt (cast_stmt);
-		      gimple_set_uid (g, gimple_uid (cast_stmt));
+		      gimple_stmt_iterator gsi = gsi_for_stmt (cast_or_tcc_cmp_stmt);
+		      gimple_set_uid (g, gimple_uid (cast_or_tcc_cmp_stmt));
 		      gimple_set_visited (g, true);
-		      gsi_insert_before (&gsi, g, GSI_SAME_STMT);
+		      gsi_insert_after (&gsi, g, GSI_SAME_STMT);
 		      FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
 			if (is_gimple_debug (use_stmt))
 			  continue;
+			else if (is_gimple_assign (use_stmt));
 			else if (gimple_code (use_stmt) == GIMPLE_COND
 				 || gimple_code (use_stmt) == GIMPLE_PHI)
 			  FOR_EACH_IMM_USE_ON_STMT (use_p, iter)