Re: Match-and-simplify and COND_EXPR

[Richard Biener <rguenther at suse dot de>]

On Mon, 10 Nov 2014, Andrew Pinski wrote:

> On Fri, Nov 7, 2014 at 2:23 AM, Richard Biener <rguenther@suse.de> wrote:
> > On Thu, 6 Nov 2014, Andrew Pinski wrote:
> >
> >> On Thu, Nov 6, 2014 at 2:40 AM, Richard Biener <rguenther@suse.de> wrote:
> >> > On Thu, 6 Nov 2014, Richard Biener wrote:
> >> >
> >> >> On Wed, 5 Nov 2014, Andrew Pinski wrote:
> >> >>
> >> >> > Hi,
> >> >> >   I was trying to hook up tree-ssa-phiopt to match-and-simplify using
> >> >> > either gimple_build (or rather using gimple_simplify depending on if
> >> >> > we want to produce cond_expr for conditional move).  I ran into a
> >> >> > problem.
> >> >> > With the pattern below:
> >> >> > /* a ? 0 : 1 -> a if 0 and 1 are integral types. */
> >> >> > (simplify
> >> >> >   (cond_expr @0 integer_zerop integer_onep)
> >> >> >   (if (INTEGRAL_TYPE_P (type))
> >> >> >     (convert @0)))
> >> >>
> >> >> Ok, so you are capturing a GENERIC expr here but nothing knows that.
> >> >> It would work if you'd do (ugh)
> >> >>
> >> >> (for op (lt le eq ne ge gt)
> >> >>  (simplify
> >> >>   (cond_expr (op @0 @1) integer_zerop integer_onep)
> >> >>   (if (INTEGRAL_TYPE_P (type))
> >> >>    (convert (op @0 @1)))))
> >> >> (simplify
> >> >>  (cond_expr SSA_NAME@0 integer_zerop integer_onep)
> >> >>   (if (INTEGRAL_TYPE_P (type))
> >> >>    (convert @0))))
> >> >>
> >> >> as a workaround.  To make your version work will require (quite)
> >> >> some special-casing in the code generator or maybe the resimplify
> >> >> helper.  Let me see if I can cook up a "simple" fix.
> >> >
> >> > Sth like below (for the real fix this has to be replicated in
> >> > all gimple_resimplifyN functions).  I'm missing a testcase
> >> > where the pattern would apply (and not be already folded by fold),
> >> > so I didn't check if it actually works.
> >>
> >> You do need to check if seq is NULL though as gimple_build depends on
> >> seq not being NULL.  But otherwise yes this works for me.
> >
> > Ok, here is a better and more complete fix.  The optimization
> > whether a captured expression may be a GENERIC condition isn't
> > implemented so a lot of checks are emitted right now.  Also
> > if gimple_build would fail this terminates the whole matching
> > process, not just matching of the current pattern (failing "late"
> > isn't supported with the style of code we emit - well, without
> > adding ugly gotos and labels).
> >
> > At least it avoids splitting up conditions if substituted into
> > a COND_EXPR, so simplifications of COND_EXPRs in-place (w/o seq)
> > should still work.
> >
> > Can you check whether this works for you?
> 
> This works for most cases but does not work for things like:
> /* a != b ? a : b -> a */
> (simplify
>   (cond_expr (ne @0 @1) @0 @1)
>   @0)
>
> In gimple_simplify after it matches COND_EXPR. It does not look into
> the operand 0 to see if it matches NE_EXPR, it just sees if it was a
> SSA_NAME.  In this case I was trying to do a simple replacement of
> value_replacement in tree-ssa-phiopt.c.

Yeah - I hoped you wouldn't notice.  It's not too difficult to fix.
Still this ugly ambiguity in the GIMPLE IL is bad and I wonder if
it is better to finally fix it with

Index: gcc/gimple-expr.c
===================================================================
--- gcc/gimple-expr.c   (revision 216973)
+++ gcc/gimple-expr.c   (working copy)
@@ -641,10 +641,7 @@ is_gimple_lvalue (tree t)
 bool
 is_gimple_condexpr (tree t)
 {
-  return (is_gimple_val (t) || (COMPARISON_CLASS_P (t)
-                               && !tree_could_throw_p (t)
-                               && is_gimple_val (TREE_OPERAND (t, 0))
-                               && is_gimple_val (TREE_OPERAND (t, 1))));
+  return is_gimple_val (t);
 }
 
 /* Return true if T is a gimple address.  */

I expect mostly fallout from passes building [VEC_]COND_EXPRs and
not gimplifying it and of course missed optimizations from the
vectorizer which will likely now try to vectorize the separate
comparisons in some maybe odd way (not sure).

I did this experiment once but didn't finish it.  Too bad :/

> But for my purpose right now this is sufficient and will be posting a
> patch which does not remove things from tree-ssa-phiopt.c just yet.

Fair enough.  I suppose for GCC 5 I will need to deal with the odd
GIMPLE IL.  At least I can consider it a "bug" and thus fix this
during stage3 ;)

Thanks,
Richard.