Finally getting back to this...
Richard Biener <richard.guent...@gmail.com> writes:
> 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...
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.
Thanks,
Richard
