This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch match.pd c c++]: Ignore results of 'shorten_compare' and move missing patterns in match.pd
- From: Kai Tietz <ktietz70 at googlemail dot com>
- To: Richard Biener <richard dot guenther at gmail dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Jeff Law <law at redhat dot com>
- Date: Tue, 15 Sep 2015 14:54:35 +0200
- Subject: Re: [patch match.pd c c++]: Ignore results of 'shorten_compare' and move missing patterns in match.pd
- Authentication-results: sourceware.org; auth=none
- References: <CAEwic4ZDJHyst_kgSgMxZLtRtHEbyFUoCS1g5CPGkf6isR8GAA at mail dot gmail dot com> <CAFiYyc0iTJNkXc7t5q0afhow6Haf=QjY0gK2R_=9eCXHmWFPRg at mail dot gmail dot com>
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.
>> + && 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.
>> + && 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'
>> + (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?
>> +
>> +
>> /* 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, and matches even if doing no real simplification.
I detected this by testing patterns (IIRC tree-ssa/vrp25.c showed
this), and moved pattern before this code.
>> -
>> +
>> +/* 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.
>> + (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 ....
> (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