diff mbox

Fix PR 53395: tree-if-conv.c not producing MAX_EXPR

Message ID CA+=Sn1kn3WFShS4NQNSteMf4R9s=QgDddQDTq1jCbxf-Z2J3xg@mail.gmail.com
State New
Headers show

Commit Message

Andrew Pinski Aug. 28, 2012, 7:09 a.m. UTC 
On Mon, May 21, 2012 at 2:56 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Sun, May 20, 2012 at 1:40 AM, Andrew Pinski <pinskia@gmail.com> wrote:
>> The problem here is that tree-if-conv.c produces COND_EXPR instead of
>> the MAX/MIN EXPRs.  When I added the expansion from COND_EXPR to
>> conditional moves, I assumes that the expressions which should have
>> been converted into MAX_EXPR/MIN_EXPR have already happened.
>>
>> This fixes the problem by having tree-if-conv fold the expression so
>> the MIN_EXPR/MAX_EXPR appears in the IR rather than COND_EXPR and the
>> expansion happens correctly to the min/max rtl rather than just
>> through the conditional move ones.
>>
>> OK?  Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> As we are unconditionally building a gimple_assign from the folding result
> you need to re-gimplify it.  The code was using build3 instead of fold_build3
> to avoid that and to make sure we create a plain COND_EXPR we can
> later CSE / simplify properly.  Generating a MAX_EXPR directly is certainly
> fine (can we always vectorize that?), but I suppose simply changing the
> code to use fold_build3 will have unwanted fallout (consider folds habit
> to insert conversions that are not requried on gimple).
>
> So I'd rather prefer to abstract the build3 (COND_EXPR,... into a
> helper function that uses fold_ternary and only if the result is an
> invariant/register or a MIN/MAX_EXPR use the result, canonicalizing
> it properly.

Here is an updated patch which does exactly that.

Note I noticed another regression dealing with my expansion for
COND_EXPR dealing with min/max and I will be submitting patches for
those issue tomorrow.

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

Thanks,
Andrew Pinski

ChangeLog:
* tree-if-conv.c (constant_or_ssa_name): New function.
(fold_build_cond_expr): New function.
(predicate_scalar_phi): Use fold_build_cond_expr instead of build3.
(predicate_mem_writes): Likewise.

Comments

Richard Biener Sept. 3, 2012, 11:43 a.m. UTC | #1 
On Tue, Aug 28, 2012 at 9:09 AM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Mon, May 21, 2012 at 2:56 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Sun, May 20, 2012 at 1:40 AM, Andrew Pinski <pinskia@gmail.com> wrote:
>>> The problem here is that tree-if-conv.c produces COND_EXPR instead of
>>> the MAX/MIN EXPRs.  When I added the expansion from COND_EXPR to
>>> conditional moves, I assumes that the expressions which should have
>>> been converted into MAX_EXPR/MIN_EXPR have already happened.
>>>
>>> This fixes the problem by having tree-if-conv fold the expression so
>>> the MIN_EXPR/MAX_EXPR appears in the IR rather than COND_EXPR and the
>>> expansion happens correctly to the min/max rtl rather than just
>>> through the conditional move ones.
>>>
>>> OK?  Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>>
>> As we are unconditionally building a gimple_assign from the folding result
>> you need to re-gimplify it.  The code was using build3 instead of fold_build3
>> to avoid that and to make sure we create a plain COND_EXPR we can
>> later CSE / simplify properly.  Generating a MAX_EXPR directly is certainly
>> fine (can we always vectorize that?), but I suppose simply changing the
>> code to use fold_build3 will have unwanted fallout (consider folds habit
>> to insert conversions that are not requried on gimple).
>>
>> So I'd rather prefer to abstract the build3 (COND_EXPR,... into a
>> helper function that uses fold_ternary and only if the result is an
>> invariant/register or a MIN/MAX_EXPR use the result, canonicalizing
>> it properly.
>
> Here is an updated patch which does exactly that.
>
> Note I noticed another regression dealing with my expansion for
> COND_EXPR dealing with min/max and I will be submitting patches for
> those issue tomorrow.
>
> OK? Bootstrap and tested on x86_64-linux-gnu.

Ok.

Thanks,
Richard.

> Thanks,
> Andrew Pinski
>
> ChangeLog:
> * tree-if-conv.c (constant_or_ssa_name): New function.
> (fold_build_cond_expr): New function.
> (predicate_scalar_phi): Use fold_build_cond_expr instead of build3.
> (predicate_mem_writes): Likewise.
diff mbox

Patch

Index: tree-if-conv.c
===================================================================
--- tree-if-conv.c	(revision 190736)
+++ tree-if-conv.c	(working copy)
@@ -307,6 +307,65 @@  fold_or_predicates (location_t loc, tree
   return fold_build2_loc (loc, TRUTH_OR_EXPR, boolean_type_node, c1, c2);
 }
 
+/* Returns true if N is either a constant or a SSA_NAME.  */
+
+static bool
+constant_or_ssa_name (tree n)
+{
+  switch (TREE_CODE (n))
+    {
+      case SSA_NAME:
+      case INTEGER_CST:
+      case REAL_CST:
+      case COMPLEX_CST:
+      case VECTOR_CST:
+	return true;
+      default:
+	return false;
+    }
+}
+
+/* Returns either a COND_EXPR or the folded expression if the folded
+   expression is a MIN_EXPR, a MAX_EXPR, an ABS_EXPR,
+   a constant or a SSA_NAME. */
+
+static tree
+fold_build_cond_expr (tree type, tree cond, tree rhs, tree lhs)
+{
+  tree rhs1, lhs1, cond_expr;
+  cond_expr = fold_ternary (COND_EXPR, type, cond,
+			    rhs, lhs);
+
+  if (cond_expr == NULL_TREE)
+    return build3 (COND_EXPR, type, cond, rhs, lhs);
+
+  STRIP_USELESS_TYPE_CONVERSION (cond_expr);
+
+  if (constant_or_ssa_name (cond_expr))
+    return cond_expr;
+
+  if (TREE_CODE (cond_expr) == ABS_EXPR)
+    {
+      rhs1 = TREE_OPERAND (cond_expr, 1);
+      STRIP_USELESS_TYPE_CONVERSION (rhs1);
+      if (constant_or_ssa_name (rhs1))
+	return build1 (ABS_EXPR, type, rhs1);
+    }
+
+  if (TREE_CODE (cond_expr) == MIN_EXPR
+      || TREE_CODE (cond_expr) == MAX_EXPR)
+    {
+      lhs1 = TREE_OPERAND (cond_expr, 0);
+      STRIP_USELESS_TYPE_CONVERSION (lhs1);
+      rhs1 = TREE_OPERAND (cond_expr, 1);
+      STRIP_USELESS_TYPE_CONVERSION (rhs1);
+      if (constant_or_ssa_name (rhs1)
+	  && constant_or_ssa_name (lhs1))
+	return build2 (TREE_CODE (cond_expr), type, lhs1, rhs1);
+    }
+  return build3 (COND_EXPR, type, cond, rhs, lhs);
+}
+
 /* Add condition NC to the predicate list of basic block BB.  */
 
 static inline void
@@ -1293,8 +1352,8 @@  predicate_scalar_phi (gimple phi, tree c
 			   || bb_postdominates_preds (bb));
 
       /* Build new RHS using selected condition and arguments.  */
-      rhs = build3 (COND_EXPR, TREE_TYPE (res),
-		    unshare_expr (cond), arg_0, arg_1);
+      rhs = fold_build_cond_expr (TREE_TYPE (res), unshare_expr (cond),
+				  arg_0, arg_1);
     }
 
   new_stmt = gimple_build_assign (res, rhs);
@@ -1554,7 +1613,7 @@  predicate_mem_writes (loop_p loop)
 	    cond = force_gimple_operand_gsi_1 (&gsi, unshare_expr (cond),
 					       is_gimple_condexpr, NULL_TREE,
 					       true, GSI_SAME_STMT);
-	    rhs = build3 (COND_EXPR, type, unshare_expr (cond), rhs, lhs);
+	    rhs = fold_build_cond_expr (type, unshare_expr (cond), rhs, lhs);
 	    gimple_assign_set_rhs1 (stmt, ifc_temp_var (type, rhs, &gsi));
 	    update_stmt (stmt);
 	  }