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.
> + && 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?
> + && 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.
> + && 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).
> + )
Closing parens always go to the previous line everywhere else.
> + (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.
> + (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).
> +
> +
> /* 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...
> -
> +
> +/* 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?
> + (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:
(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" } } */
