Re: Extend tree code folds to IFN_COND_*

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Jul 4, 2018 at 8:46 PM Richard Sandiford
<richard.sandiford@linaro.org> wrote:
>
> Finally getting back to this...
>
> Richard Biener <richard.guenther@gmail.com> writes:
> > On Wed, Jun 6, 2018 at 10:16 PM Richard Sandiford
> > <richard.sandiford@linaro.org> wrote:
> >>
> >> > On Thu, May 24, 2018 at 11:36 AM Richard Sandiford
> >> > <richard.sandiford@linaro.org> wrote:
> >> >>
> >> >> This patch adds match.pd support for applying normal folds to their
> >> >> IFN_COND_* forms.  E.g. the rule:
> >> >>
> >> >>   (plus @0 (negate @1)) -> (minus @0 @1)
> >> >>
> >> >> also allows the fold:
> >> >>
> >> >>   (IFN_COND_ADD @0 @1 (negate @2) @3) -> (IFN_COND_SUB @0 @1 @2 @3)
> >> >>
> >> >> Actually doing this by direct matches in gimple-match.c would
> >> >> probably lead to combinatorial explosion, so instead, the patch
> >> >> makes gimple_match_op carry a condition under which the operation
> >> >> happens ("cond"), and the value to use when the condition is false
> >> >> ("else_value").  Thus in the example above we'd do the following
> >> >>
> >> >> (a) convert:
> >> >>
> >> >>       cond:NULL_TREE (IFN_COND_ADD @0 @1 @4 @3) else_value:NULL_TREE
> >> >>
> >> >>     to:
> >> >>
> >> >>       cond:@0 (plus @1 @4) else_value:@3
> >> >>
> >> >> (b) apply gimple_resimplify to (plus @1 @4)
> >> >>
> >> >> (c) reintroduce cond and else_value when constructing the result.
> >> >>
> >> >> Nested operations inherit the condition of the outer operation
> >> >> (so that we don't introduce extra faults) but have a null else_value.
> >> >> If we try to build such an operation, the target gets to choose what
> >> >> else_value it can handle efficiently: obvious choices include one of
> >> >> the operands or a zero constant.  (The alternative would be to have some
> >> >> representation for an undefined value, but that seems a bit invasive,
> >> >> and isn't likely to be useful here.)
> >> >>
> >> >> I've made the condition a mandatory part of the gimple_match_op
> >> >> constructor so that it doesn't accidentally get dropped.
> >> >>
> >> >> Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf
> >> >> and x86_64-linux-gnu.  OK to install?
> >> >
> >> > It looks somewhat clever but after looking for a while it doesn't handle
> >> > simplifying
> >> >
> >> >  (IFN_COND_ADD @0 @1 (IFN_COND_SUB @0 @2 @1 @3) @3)
> >> >
> >> > to
> >> >
> >> >  (cond @0 @2 @3)
> >> >
> >> > right?  Because while the conditional gimple_match_op is built
> >> > by try_conditional_simplification it isn't built when doing
> >> > SSA use->def following in the generated matching code?
> >>
> >> Right.  This would be easy to add, but there's no motivating case yet.
> >
> > ...
> >
> >> > So it looks like a bit much noise for this very special case?
> >> >
> >> > I suppose you ran into the need of these foldings from looking
> >> > at real code - which foldings specifically were appearing here?
> >> > Usually code is well optimized before if-conversion/vectorization
> >> > so we shouldn't need full-blown handling?
> >>
> >> It's needed to get the FMA, FMS, FNMA and FNMS folds for IFN_COND_* too.
> >> I thought it'd be better to do it "automatically" rather than add specific
> >> folds, since if we don't do it automatically now, it's going to end up
> >> being a precedent for not doing it automatically in future either.
> >
> > ... not like above isn't a similar precedent ;)  But OK, given...
>
> But we're not doing the above case manually either yet :-)  Whereas the
> series does need to do what the patch does one way or another.
>
> Also, it might be hard to do the above case manually anyway (i.e. match
> nested IFN_COND_* ops with an implicitly-conditional top-level op),
> since the match.pd rule wouldn't have easy access to the overall condition.
> And that's by design, or so I'd like to claim.
>
> >> > That said, I'm not sure how much work it is to massage
> >> >
> >> >       if (gimple *def_stmt = get_def (valueize, op2))
> >> >         {
> >> >           if (gassign *def = dyn_cast <gassign *> (def_stmt))
> >> >             switch (gimple_assign_rhs_code (def))
> >> >               {
> >> >               case PLUS_EXPR:
> >> >
> >> > to look like
> >> >
> >> >       if (gimple *def_stmt = get_def (valueize, op2))
> >> >         {
> >> >            code = ERROR_MARK;
> >> >            if (!is_cond_ifn_with_cond (curr_gimple_match_op, &code))
> >> >              if (gassign *def dyn_cast <gassign *> (def_stmt))
> >> >                code = gimple_assign_rhs_code (def);
> >> >            switch (code)
> >> >              {
> >> >              case PLUS_EXPR:
> >> >
> >> > thus transparently treat the IFN_COND_* as their "code" if the condition
> >> > matches that of the context (I'm not sure if we can do anything for
> >> > mismatching contexts).
> >>
> >> Yeah, this was one approach I had in mind for the subnodes, if we do
> >> end up needing it.  But at least for the top-level node, we want to try
> >> both as a native IFN_COND_FOO and as a conditional FOO, which is why the
> >> top-level case is handled directly in gimple-match-head.c.
> >>
> >> Of course, trying both for subnodes would lead to exponential behaviour
> >> in general.  And like you say, in practice most double-IFN_COND cases
> >> should have been folded before we created the IFN_CONDs, so it's hard
> >> to tell which recursive behaviour would be best.
> >
> > ... this it probably makes sense to do it "simple" first.
> >
> > Btw, I'd simply _only_ consider the IFN_ stripped path when looking for
> > defs if it has matching condition (which is a prerequesite anyway).  So
> > the get_at_the_def () would first check for IFN_ with matching condition
> > and then expand to the unconditional operation.  And get_at_the_def ()
> > for UNCOND context would never look into IFN_s.
>
> Yeah, agree we should only consider stripping the conditional part if
> the conditions (and perhaps also the "else" values) are the same.
> But I don't know whether we should *only* consider the unconditional
> part in that case.  (For one thing it would mean we'd still try to the
> match the IFN form whenever the conditions don't match, which might
> might be surprising.)
>
> This is why I'd prefer to wait until we have a motivating case rather
> than try to decide now.
>
> > Even the toplevel handling probably never will need the outermost
> > conditional IFN_ handling - at least I can't think of a pattern that
> > you would be forced to write the outermost conditional IFN_
> > explicitely?
>
> We might want stuff like:
>
>  (simplify
>   (IFN_COND_ADD @0 @1 @2 (plus @1 @3))
>   (plus @1 (vec_cond @0 @2 @3)))
>
> Not sure how useful that would be in practice, but it seems at least
> plausible.

Interesting, yeah, that looks plausible.

So let's go with the patch for now.

Richard.

>
> Thanks,
> Richard