Re: [patch match.pd c c++]: Ignore results of 'shorten_compare' and move missing patterns in match.pd

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

On Tue, Sep 15, 2015 at 2:54 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> Hi Richi,
>
> thanks for the review.
>
> 2015-09-15 12:06 GMT+02:00 Richard Biener <richard.guenther@gmail.com>:
>> On Tue, Sep 8, 2015 at 1:17 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>> Hi,
>>>
>>> This patch is the first part of obsoleting 'shorten_compare' function
>>> for folding.
>>> It adjusts the uses of 'shorten_compare' to ignore folding returned by
>>> it, and adds
>>> missing pattterns to match.pd to allow full bootstrap of C/C++ without
>>> regressions.
>>> Due we are using 'shorten_compare' for some diagnostic we can't simply
>>> remove it.  So if this patch gets approved, the next step will be to
>>> rename the function to something like 'check_compare', and adjust its
>>> arguments and inner logic to reflect that we don't modify
>>> arguments/expression anymore within that function.
>>>
>>> Bootstrap just show 2 regressions within gcc.dg testsuite due patterns
>>> matched are folded more early by forward-propagation.  I adjusted
>>> them, and added them to patch, too.
>>
>> Some comments on the patterns you added below
>>
>>> I did regression-testing for x86_64-unknown-linux-gnu.
>>>
>>> ChangeLog
>>>
>>> 2015-09-08  Kai Tietz  <ktietz@redhat.com>
>>>
>>>     * match.pd: Add missing patterns from shorten_compare.
>>>     * c/c-typeck.c (build_binary_op): Discard foldings of shorten_compare.
>>>     * cp/typeck.c (cp_build_binary_op): Likewise.
>>>
>>> 2015-09-08  Kai Tietz  <ktietz@redhat.com>
>>>
>>>     * gcc.dg/tree-ssa/vrp23.c: Adjust testcase to reflect that
>>>     pattern is matching now already within forward-propagation pass.
>>>     * gcc.dg/tree-ssa/vrp24.c: Likewise.
>>>
>>> Index: match.pd
>>> ===================================================================
>>> --- match.pd    (Revision 227528)
>>> +++ match.pd    (Arbeitskopie)
>>> @@ -1786,6 +1786,45 @@ along with GCC; see the file COPYING3.  If not see
>>>    (op (abs @0) zerop@1)
>>>    (op @0 @1)))
>>>
>>> +/* Simplify '((type) X) cmp ((type) Y' to shortest possible types, of X and Y,
>>> +   if type's precision is wider then precision of X's and Y's type.
>>> +   Logic taken from shorten_compare function.  */
>>> +(for op (tcc_comparison)
>>> +  (simplify
>>> +    (op (convert@0 @1) (convert@3 @2))
>>> +    (if ((TREE_CODE (TREE_TYPE (@1)) == REAL_TYPE)
>>> +         == (TREE_CODE (TREE_TYPE (@2)) == REAL_TYPE)
>>> +         && (TREE_CODE (TREE_TYPE (@1)) == REAL_TYPE)
>>> +            == (TREE_CODE (TREE_TYPE (@0)) == REAL_TYPE)
>>
>> I think these are always true, to/from REAL_TYPE is FIX_TRUNC /
>> FLOAT_EXPR.  What you
>> might get is conversion to/from decimal FP from/to non-decimal FP.
>
> Right, I just wanted to handle patterns of shorten_compare here.  I
> agree that - with small adjustments - we can use same logic for
> FIX_TRUNC and FLOAT_EXPR, too.

I was saying the test is useless, not that you should handle
FIX_TRUNC/FLOAT_EXPR.

>>> +         && single_use (@1)
>>> +         && single_use (@3)
>>
>> We have :s now, I'd like to see comments before any explicit
>> single_use () uses as to why
>> they were added.  Also why @1 and @3?  Either @0 and @3 or @1 and @2?
>
> Thanks for pointing me to :s.  I removedfrom updated patch the @3.
> Reason for introducing it was just for checking if expression has
> single-use, as otherwise we don't want to split expression by this
> transformation.
>
>>> +         && TYPE_UNSIGNED (TREE_TYPE (@1)) == TYPE_UNSIGNED (TREE_TYPE (@2))
>>> +         && !((TREE_CODE (TREE_TYPE (@1)) == REAL_TYPE
>>> +               && DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (@1))))
>>> +              || (TREE_CODE (TREE_TYPE (@2)) == REAL_TYPE
>>> +                  && DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (@2)))))
>>
>> We have DECIMAL_FLOAT_TYPE_P () for this.
>
> DECIMAL_FLOAT_TYPE seems to check for wide kind of patterns AFAICS.
> It uses internall SCALAR_FLOAT_TYPE_P,
> which also checks for COMPLEX_TYPE, not just for REAL_TYPE.  I  code
> in shorten_compare, which rejects DECIMAL_FLOAT_MODE_P just for
> REAL_TYPEs.
>
> But well, not sure if this is a latent bug in shorten_compare ...  so
> I replaced code using DECIMAL_FLOAT_TYPE instead.

I'd say it's a latent bug in shorten_compare.

>>> +         && TYPE_PRECISION (TREE_TYPE (@1)) < TYPE_PRECISION (TREE_TYPE (@0))
>>> +         && TYPE_PRECISION (TREE_TYPE (@2)) < TYPE_PRECISION (TREE_TYPE (@0))
>>
>> copy-paste error, @3 (ok, it shouldn't really matter as @0 and @3
>> should be the same types).
>
> Hmm, no pasto.  Initially (as told above) I didn't used @3 as it seems
> to be pretty superflous here.
> You are right that it doesn't matter as convert@0 and convert@3 should
> be always the same type.
>
>>> +         )
>>
>> Closing parens always go to the previous line everywhere else.
>
> Ok. It just looks to me less good readable without skilled editor
>
>>> +       (with {
>>> +         tree comtype = TYPE_PRECISION (TREE_TYPE (@1))
>>> +                 < TYPE_PRECISION (TREE_TYPE (@2)) ? TREE_TYPE (@2)
>>> +                                   : TREE_TYPE (@1);
>>> +         if (INTEGRAL_TYPE_P (TREE_TYPE (@0)))
>>> +           {
>>> +             if (TYPE_UNSIGNED (TREE_TYPE (@1)) || TYPE_UNSIGNED
>>> (TREE_TYPE (@0)))
>>> +               comtype = unsigned_type_for (comtype);
>>> +             else
>>> +               comtype = signed_type_for (comtype);
>>> +           }
>>> +         }
>>
>> I think fold-const.c or tree.c has a helper for this, if not we should
>> add one.  I suppose
>> you copied the above from some frontend code which should also use that helper.
>
> We have for C/C++ FEs the function common_type, which does something
> equivalent (well, much more specialized then we would want here).
> I would be wrong to call this here in match.pd.  There seems to be no
> such code AFAICS, and it is a transition from C-FEs specifc code in
> shorten_compare.  So no idea where we are using elsewhere such
> pattern.
> Anyway, we can add a helper-functions to tree.c 'common_type_for'

That would be nice.

>>> +            (op (convert:comtype @1) (convert:comtype @2))
>>> +       )
>>> +     )
>>> +  )
>>> +)
>>
>> See above for closing parens.  I wonder if we can't merge the above
>> with the existing
>> simplification of widened compares which also "merges" the pattern you add for
>> the 2nd operand not being converted (the constant case).
>
> Here I weren't sure about pattern to be used for having multiple
> branches.  Is "switch" intended for this?

What do you mean with "multiple branches"?  Yes, the current pattern has
one outer if and a then and else clause.  If the if-structure would look like
if () {} else if () {} else if () {} then this is what switch is for.

>>> +
>>> +
>>>  /* From fold_sign_changed_comparison and fold_widened_comparison.  */
>>>  (for cmp (simple_comparison)
>>>   (simplify
>>> @@ -2046,7 +2085,43 @@ along with GCC; see the file COPYING3.  If not see
>>>          (if (cmp == LE_EXPR)
>>>       (ge (convert:st @0) { build_zero_cst (st); })
>>>       (lt (convert:st @0) { build_zero_cst (st); }))))))))))
>>
>> at least it's odd you place your patterns before and after this existing one...
>
> True, initially I had all this patterns below existing one.
> Nevertheless placement isn't odd.  Pattern here is same as prior
> pattern

which is why I suggest to merge them...

>, and matches even if doing no real simplification.

it's true that this can happen (I didn't bother to fix it ... eh), but then your
ordering will cause the same issue just the other way around.  Btw, I don't
see how it can happen from looking at generic-match.c without your patch.

> I detected this by testing patterns (IIRC tree-ssa/vrp25.c showed
> this), and moved pattern before this code.

So is it that both patterns match and both do some transform just not
the same one?

Well, the patterns are so closely related they should be merged to do
the best transform, not the one that matches first.

>
>>> -
>>> +
>>> +/* Simplify '(type) X cmp CST' to 'X cmp (type-of-X) CST', if
>>> +   CST fits into the type of X.  */
>>> +(for cmp (simple_comparison)
>>> +  (simplify
>>> +    (cmp (convert@2 @0) INTEGER_CST@1)
>>
>> what about REAL_CSTs?
>
> Well, for fast-math this might be something to be extended.  So I
> handled here just the common variant.

So shorten_compare doesn't do that yet?  Ok...

I was thinking about (double)x < 1.0

with float x where 1.0 is exactly representable as float (I think even for
not exactly representable constants we might be able to do the shortening).

We can do that as followup if shorten_compare doesn't do this.  Btw, there
are a load of this kind of transforms in convert.c still ...

>>> +    (if (INTEGRAL_TYPE_P (TREE_TYPE (@0))
>>> +         && TYPE_PRECISION (TREE_TYPE (@1)) > TYPE_PRECISION (TREE_TYPE (@0))
>>> +         && single_use (@2)
>>> +         && (TYPE_UNSIGNED (TREE_TYPE (@0))
>>> +             || TYPE_UNSIGNED (TREE_TYPE (@0)) == TYPE_UNSIGNED
>>> (TREE_TYPE (@1))
>>> +             || cmp == NE_EXPR || cmp == EQ_EXPR)
>>> +         && !POINTER_TYPE_P (TREE_TYPE (@0))
>>> +         && int_fits_type_p (@1, TREE_TYPE (@0)))
>>> +      (with { tree optype = TREE_TYPE (@0); }
>>> +        (cmp @0 (convert:optype @1))
>>> +      )
>>> +    )
>>> +  )
>>> +)
>>> +
>>> +/* See if '(type) X ==/!= CST' represents a condition,
>>> +   which is always true, or false due CST's value.  */
>>> +(for cmp (ne eq)
>>> +  (simplify
>>> +    (cmp (convert@2 @0) INTEGER_CST@1)
>>> +    (if (INTEGRAL_TYPE_P (TREE_TYPE (@0))
>>> +         && TYPE_PRECISION (TREE_TYPE (@1)) >= TYPE_PRECISION (TREE_TYPE (@0))
>>> +         && single_use (@2)
>>> +         && !POINTER_TYPE_P (TREE_TYPE (@0))
>>> +         && !int_fits_type_p (@1, TREE_TYPE (@0))
>>> +         && TYPE_UNSIGNED (TREE_TYPE (@0)) == TYPE_UNSIGNED (TREE_TYPE (@1)))
>>> +      { constant_boolean_node (cmp == NE_EXPR, type); }
>>> +    )
>>> +  )
>>> +)
>>
>> The existing pattern should already handle this even for other comparison codes:
>
> Yes, this pattern puzzled me too, as we deal in front with such cases
> for type-min/max already.  But for some reason I had to explicit add
> it.  I will recheck this addition, as it should be coverted by prior
> patterns ....

It might make sense to split this patch into individual pieces.  When
I move patterns from
fold I always leave the fold code in but with a gcc_unreachable
transform to test if it
still triggers after adding the match.pd variant.  You can do the same
for shorten_compare.

Richard.

>>       (if (TREE_CODE (@10) == INTEGER_CST
>>            && TREE_CODE (TREE_TYPE (@00)) == INTEGER_TYPE
>>            && !int_fits_type_p (@10, TREE_TYPE (@00)))
>>        (with
>>         {
>>           tree min = lower_bound_in_type (TREE_TYPE (@10), TREE_TYPE (@00));
>>           tree max = upper_bound_in_type (TREE_TYPE (@10), TREE_TYPE (@00));
>>           bool above = integer_nonzerop (const_binop (LT_EXPR, type, max, @10));
>>           bool below = integer_nonzerop (const_binop (LT_EXPR, type, @10, min));
>>         }
>>         (if (above || below)
>>          (if (cmp == EQ_EXPR || cmp == NE_EXPR)
>>           { constant_boolean_node (cmp == EQ_EXPR ? false : true, type); }
>>           (if (cmp == LT_EXPR || cmp == LE_EXPR)
>>            { constant_boolean_node (above ? true : false, type); }
>>            (if (cmp == GT_EXPR || cmp == GE_EXPR)
>>             { constant_boolean_node (above ? false : true, type); }))))))))))))
>>
>> Richard.
>>
>>> +
>>>  (for cmp (unordered ordered unlt unle ungt unge uneq ltgt)
>>>   /* If the second operand is NaN, the result is constant.  */
>>>   (simplify
>>> Index: cp/typeck.c
>>> ===================================================================
>>> --- cp/typeck.c    (Revision 227528)
>>> +++ cp/typeck.c    (Arbeitskopie)
>>> @@ -5000,14 +5000,8 @@ cp_build_binary_op (location_t location,
>>>           pass the copies by reference, then copy them back afterward.  */
>>>        tree xop0 = op0, xop1 = op1, xresult_type = result_type;
>>>        enum tree_code xresultcode = resultcode;
>>> -      tree val
>>> -        = shorten_compare (location, &xop0, &xop1, &xresult_type,
>>> -                   &xresultcode);
>>> -      if (val != 0)
>>> -        return cp_convert (boolean_type_node, val, complain);
>>> -      op0 = xop0, op1 = xop1;
>>> -      converted = 1;
>>> -      resultcode = xresultcode;
>>> +      shorten_compare (location, &xop0, &xop1, &xresult_type,
>>> +               &xresultcode);
>>>      }
>>>
>>>        if ((short_compare || code == MIN_EXPR || code == MAX_EXPR)
>>> Index: c/c-typeck.c
>>> ===================================================================
>>> --- c/c-typeck.c    (Revision 227528)
>>> +++ c/c-typeck.c    (Arbeitskopie)
>>> @@ -11193,20 +11193,9 @@ build_binary_op (location_t location, enum tree_co
>>>           pass the copies by reference, then copy them back afterward.  */
>>>        tree xop0 = op0, xop1 = op1, xresult_type = result_type;
>>>        enum tree_code xresultcode = resultcode;
>>> -      tree val
>>> -        = shorten_compare (location, &xop0, &xop1, &xresult_type,
>>> -                   &xresultcode);
>>> +      shorten_compare (location, &xop0, &xop1, &xresult_type,
>>> +               &xresultcode);
>>>
>>> -      if (val != 0)
>>> -        {
>>> -          ret = val;
>>> -          goto return_build_binary_op;
>>> -        }
>>> -
>>> -      op0 = xop0, op1 = xop1;
>>> -      converted = 1;
>>> -      resultcode = xresultcode;
>>> -
>>>        if (c_inhibit_evaluation_warnings == 0)
>>>          {
>>>            bool op0_maybe_const = true;
>>> Index: gcc.dg/tree-ssa/vrp23.c
>>> ===================================================================
>>> --- gcc.dg/tree-ssa/vrp23.c    (Revision 227528)
>>> +++ gcc.dg/tree-ssa/vrp23.c    (Arbeitskopie)
>>> @@ -1,5 +1,5 @@
>>>  /* { dg-do compile } */
>>> -/* { dg-options "-O2 -fdump-tree-vrp1-details" } */
>>> +/* { dg-options "-O2 -fdump-tree-vrp1-details -fdump-tree-forwprop1" } */
>>>
>>>  void aa (void);
>>>  void aos (void);
>>> @@ -44,6 +44,10 @@ L8:
>>>
>>>  /* The n_sets > 0 test can be simplified into n_sets == 1 since the
>>>     only way to reach the test is when n_sets <= 1, and the only value
>>> -   which satisfies both conditions is n_sets == 1.  */
>>> -/* { dg-final { scan-tree-dump-times "Simplified relational" 1 "vrp1" } } */
>>> +   which satisfies both conditions is n_sets == 1.
>>> +   We catch this pattern for this testcase already in forward-propagation
>>> +   pass, as use of n_sets becomes single one.  Therefore we expect that
>>> +   vrp won't find this pattern anymore.  */
>>> +/* { dg-final { scan-tree-dump-times "Simplified relational" 0 "vrp1" } } */
>>> +/* { dg-final { scan-tree-dump-times "Replaced" 3 "forwprop1" } } */
>>>
>>> Index: gcc.dg/tree-ssa/vrp24.c
>>> ===================================================================
>>> --- gcc.dg/tree-ssa/vrp24.c    (Revision 227528)
>>> +++ gcc.dg/tree-ssa/vrp24.c    (Arbeitskopie)
>>> @@ -1,5 +1,5 @@
>>>  /* { dg-do compile } */
>>> -/* { dg-options "-O2 -fdump-tree-vrp1-details" } */
>>> +/* { dg-options "-O2 -fdump-tree-vrp1-details -fdump-tree-forwprop1"  } */
>>>
>>>
>>>  struct rtx_def;
>>> @@ -90,6 +90,9 @@ L7:
>>>
>>>     The second n_sets > 0 test can also be simplified into n_sets == 1
>>>     as the only way to reach the tests is when n_sets <= 1 and the only
>>> -   value which satisfies both conditions is n_sets == 1.  */
>>> -/* { dg-final { scan-tree-dump-times "Simplified relational" 2 "vrp1" } } */
>>> +   value which satisfies both conditions is n_sets == 1.
>>> +   We catch one of the simplifications already in forward-propagation pass.
>>> +   Therefore we exprect just one simplified relational detected
>>> within vrp1.  */
>>> +/* { dg-final { scan-tree-dump-times "Simplified relational" 1 "vrp1" } } */
>>> +/* { dg-final { scan-tree-dump-times "Replaced" 2 "forwprop1" } } */
>
> Kai
>
> Updated patch part for match.pd so far>
>
> Index: match.pd
> ===================================================================
> --- match.pd    (Revision 227752)
> +++ match.pd    (Arbeitskopie)
> @@ -1786,6 +1786,36 @@ along with GCC; see the file COPYING3.  If not see
>    (op (abs @0) zerop@1)
>    (op @0 @1)))
>
> +/* Simplify '((type) X) cmp ((type) Y' to shortest possible types, of X and Y,
> +   if type's precision is wider then precision of X's and Y's type.
> +   Logic taken from shorten_compare function.  */
> +(for op (tcc_comparison)
> +  (simplify
> +    (op (convert@0:s @1) (convert:s @2))
> +    (if ((TREE_CODE (TREE_TYPE (@1)) == REAL_TYPE)
> +         == (TREE_CODE (TREE_TYPE (@2)) == REAL_TYPE)
> +         && (TREE_CODE (TREE_TYPE (@1)) == REAL_TYPE)
> +            == (TREE_CODE (TREE_TYPE (@0)) == REAL_TYPE)
> +         && TYPE_UNSIGNED (TREE_TYPE (@1)) == TYPE_UNSIGNED (TREE_TYPE (@2))
> +         && !DECIMAL_FLOAT_TYPE_P (TREE_TYPE (@1))
> +         && !DECIMAL_FLOAT_TYPE_P (TREE_TYPE (@2))
> +         && TYPE_PRECISION (TREE_TYPE (@1)) < TYPE_PRECISION (TREE_TYPE (@0))
> +         && TYPE_PRECISION (TREE_TYPE (@2)) < TYPE_PRECISION (TREE_TYPE (@0)))
> +       (with {
> +         tree comtype = TYPE_PRECISION (TREE_TYPE (@1))
> +                 < TYPE_PRECISION (TREE_TYPE (@2)) ? TREE_TYPE (@2)
> +                                   : TREE_TYPE (@1);
> +         if (INTEGRAL_TYPE_P (TREE_TYPE (@0)))
> +           {
> +             if (TYPE_UNSIGNED (TREE_TYPE (@1)) || TYPE_UNSIGNED
> (TREE_TYPE (@0)))
> +               comtype = unsigned_type_for (comtype);
> +             else
> +               comtype = signed_type_for (comtype);
> +           }
> +         }
> +            (op (convert:comtype @1) (convert:comtype @2))))))
> +
> +
>  /* From fold_sign_changed_comparison and fold_widened_comparison.  */
>  (for cmp (simple_comparison)
>   (simplify
> @@ -2046,7 +2076,41 @@ along with GCC; see the file COPYING3.  If not see
>          (if (cmp == LE_EXPR)
>       (ge (convert:st @0) { build_zero_cst (st); })
>       (lt (convert:st @0) { build_zero_cst (st); }))))))))))
> -
> +
> +/* Simplify '(type) X cmp CST' to 'X cmp (type-of-X) CST', if
> +   CST fits into the type of X.  */
> +(for cmp (simple_comparison)
> +  (simplify
> +    (cmp (convert:s @0) INTEGER_CST@1)
> +    (if (INTEGRAL_TYPE_P (TREE_TYPE (@0))
> +         && TYPE_PRECISION (TREE_TYPE (@1)) > TYPE_PRECISION (TREE_TYPE (@0))
> +         && (TYPE_UNSIGNED (TREE_TYPE (@0))
> +             || TYPE_UNSIGNED (TREE_TYPE (@0)) == TYPE_UNSIGNED
> (TREE_TYPE (@1))
> +             || cmp == NE_EXPR || cmp == EQ_EXPR)
> +         && !POINTER_TYPE_P (TREE_TYPE (@0))
> +         && int_fits_type_p (@1, TREE_TYPE (@0)))
> +      (with { tree optype = TREE_TYPE (@0); }
> +        (cmp @0 (convert:optype @1))
> +      )
> +    )
> +  )
> +)
> +
> +/* See if '(type) X ==/!= CST' represents a condition,
> +   which is always true, or false due CST's value.  */
> +(for cmp (ne eq)
> +  (simplify
> +    (cmp (convert:s @0) INTEGER_CST@1)
> +    (if (INTEGRAL_TYPE_P (TREE_TYPE (@0))
> +         && TYPE_PRECISION (TREE_TYPE (@1)) >= TYPE_PRECISION (TREE_TYPE (@0))
> +         && !POINTER_TYPE_P (TREE_TYPE (@0))
> +         && !int_fits_type_p (@1, TREE_TYPE (@0))
> +         && TYPE_UNSIGNED (TREE_TYPE (@0)) == TYPE_UNSIGNED (TREE_TYPE (@1)))
> +      { constant_boolean_node (cmp == NE_EXPR, type); }
> +    )
> +  )
> +)
> +
>  (for cmp (unordered ordered unlt unle ungt unge uneq ltgt)
>   /* If the second operand is NaN, the result is constant.  */
>   (simplify