On Wed, Jun 6, 2018 at 10:16 PM Richard Sandiford
<richard.sandif...@linaro.org> wrote:
>
> > On Thu, May 24, 2018 at 11:36 AM Richard Sandiford
> > <richard.sandif...@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...
> > 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.
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?
So maybe modify your patch to never try both toplevel variants either.
Thanks,
Richard.
> Thanks,
> Richard
