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

login
register
mail settings
Submitter Andrew Pinski
Date May 19, 2012, 11:40 p.m.
Message ID <CA+=Sn1mcw_JpLWxE6r50so6e6xcrfnYpMGCtGxDTsi3NcJ4r7Q@mail.gmail.com>
Download mbox | patch
Permalink /patch/160218/
State New
Headers show

Comments

Andrew Pinski - May 19, 2012, 11:40 p.m.
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.

Thanks,
Andrew Pinski

ChangeLog:
* tree-if-conv.c (predicate_scalar_phi): Call fold_build3 instead of build3.
(predicate_mem_writes): Likewise.
Richard Guenther - May 21, 2012, 9:56 a.m.
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.

Richard.

> Thanks,
> Andrew Pinski
>
> ChangeLog:
> * tree-if-conv.c (predicate_scalar_phi): Call fold_build3 instead of build3.
> (predicate_mem_writes): Likewise.

Patch

Index: tree-if-conv.c
===================================================================
--- tree-if-conv.c	(revision 187683)
+++ tree-if-conv.c	(working copy)
@@ -1313,8 +1313,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_build3 (COND_EXPR, TREE_TYPE (res),
+		         unshare_expr (cond), arg_0, arg_1);
     }
 
   new_stmt = gimple_build_assign (res, rhs);
@@ -1574,7 +1574,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_build3 (COND_EXPR, type, unshare_expr (cond), rhs, lhs);
 	    gimple_assign_set_rhs1 (stmt, ifc_temp_var (type, rhs, &gsi));
 	    update_stmt (stmt);
 	  }