Hi Richi, thanks for the review.
2015-09-15 12:06 GMT+02:00 Richard Biener <richard.guent...@gmail.com>: > On Tue, Sep 8, 2015 at 1:17 PM, Kai Tietz <ktiet...@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 <kti...@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 <kti...@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