Re: Move ABS detection from fold-const.c to match.pd

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

On Sun, Jun 28, 2015 at 8:34 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> (this message looks like it was lost in my draft folder...)
>
> On Tue, 26 May 2015, Richard Biener wrote:
>
>> +(match zerop integer_zerop)
>> +(match zerop real_zerop)
>>
>> Would it also include fixed_zerop?
>
>
> Probably, yes. The main issue is that I know next to nothing about
> fixed-point types, so I am always unsure how to handle them (when I don't
> forget them completely). For instance, in the recently added -A CMP -B, we
> could probably replace
>
>   (if (FLOAT_TYPE_P (TREE_TYPE (@0))
>        || (ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
>            && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@0))))
>
> with
>
>   (if (FLOAT_TYPE_P (TREE_TYPE (@0))
>        || TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@0)))

Not sure if TYPE_OVERFLOW_UNDEFINED says sth sensible for fixed-point
types given that there is no overflow for them but they saturate.  As far as I
see the check would even ICE without guarding it with
ANY_INTEGRAL_TYPE_P.  So it would be

   || NON_SAT_FIXED_POINT_TYPE_P (TREE_TYPE (@0))

where I am not sure whether overflow is undefined for non-saturating fixed-point
types ...

>
>> Note that with inlining implemented it would duplicate the pattern for
>> each match variant thus in this case adding a tree.[ch] function zerop ()
>> might be better.
>
>
> Ah... I actually thought we might end up moving things like integer_zerop
> from tree.c to match.pd, especially since predicates are not declared
> 'static'... Ok, reverse gear.

Yeah, I don't think match.pd is a good fit for them.

> Note that inlining does not seem necessary to implement more advanced
> predicates like negated_value_for_comparison in the parent message.

Sure not necessary but one point of match-and-simplify was that
the pattern matching is fast because it uses a decision tree.  Once
you introduce predicates that are in functions with their own decision
tree you get back to testing all of them.

>> +   (simplify
>> +    (cnd (cmp @0 zerop) (convert?@2 @0) (negate@1 @2))
>> +    (if (cmp == EQ_EXPR || cmp == UNEQ_EXPR)
>> +     @1)
>> +    (if (cmp == NE_EXPR || cmp == LTGT_EXPR)
>> +     (non_lvalue @2))
>> +    (if (TYPE_SIGN (TREE_TYPE (@0)) == SIGNED /* implicit */
>> +        && TYPE_SIGN (type) == SIGNED
>> +        && element_precision (type) >= element_precision (TREE_TYPE
>> (@0)))
>> +     (if (cmp == GE_EXPR || cmp == GT_EXPR
>> +         || (!flag_trapping_math && (cmp == UNGE_EXPR || cmp ==
>> UNGT_EXPR)))
>> +      (abs @2))
>> +     (if (cmp == LE_EXPR || cmp == LT_EXPR
>> +         || (!flag_trapping_math && (cmp == UNLE_EXPR || cmp ==
>> UNLT_EXPR)))
>> +      (negate (abs @2)))))
>> +   /* Now with the branches swapped.  */
>> +   (simplify
>> +    (cnd (cmp @0 zerop) (negate@1 (convert?@2 @0)) @2)
>>
>> not obvious from a quick look - but would you be able to remove the
>> swapped branch
>> vairant if (cnd:c (cmp @0 zerop) X Y) would work by swapping X and Y?
>
>
> Hmm. How do I test if I am currently in the original or commuted version of
> the simplification?

You can't.

> I could add a "with" block that defines truecmp as
> either cmp or invert_tree_comparison (cmp) and test that. Otherwise, I would
> need a test before each "return" as swapped versions don't return the same
> thing. It might make a slight difference on the handling of
> flag_trapping_math, but that handling already seems strange to me...

(cnd:c (cmp @0 zerop) (convert?@2 @0) (negate@1 @2))

would get you

 (cnd (cmp @0 zerop) (convert?@2 @0) (negate@1 @2))

and

 (cnd (cmp @0 zerop) (negate@1 @2) (convert?@2 @0))

in the patterns it almost literally looked like what you did manually.

>> The fold-const.c code doesn't seem to handle as many variants (esp.
>> the swapping?),
>
>
> The fold-const.c function is called twice, once on regular operands, once
> with inverted comparison and swapped operands. I really don't think I am
> handling more cases (except maybe the silly a?a:0 is extended to unsigned).

Ok.

>> so maybe you can add a testcase that exercises some of the above on
>> GIMPLE?
>
>
> So mostly the VEC_COND_EXPR version? We don't seem to have that much
> COND_EXPR left in gimple.

Ah, true.  Yes, the vector variant then.

Thanks,
Richard.

> --
> Marc Glisse