Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons
On 07/03/2017 12:59 PM, Marc Glisse wrote: >> What happens if @0 is a floating point type? Based on the variable name >> "itype" and passing TYPE_PRECISION (itype) to real_to_integer, it seems >> like you're expecting @0 to be an integer. If so, you should verify >> that it really is an integer type. Seems like a good thing to verify >> with tests as well. > > @0 is the argument of a FLOAT_EXPR. verify_gimple_assign_unary > guarantees that it is INTEGRAL_TYPE_P (or VECTOR_INTEGER_TYPE_P but then > the result would have to be VECTOR_FLOAT_TYPE_P, and since it gets > compared to REAL_CST... the test SCALAR_FLOAT_TYPE_P is actually > redundant). Duh. I should have realized that. My bad. jeff
Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons
On Mon, 3 Jul 2017, Jeff Law wrote: On 07/02/2017 11:03 AM, Yuri Gribov wrote: Hi all, This is initial patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 . Per Joseph's suggestion it optimizes (float)lhs CMP rhs (double)lhs CMP rhs to lhs CMP (typeof(x))rhs whenever typeof(x) can be precisely represented by floating-point type (e.g. short by float or int by double) and rhs can be precisely represented by typeof(x). Bootstrapped/regtested on x64. Ok for trunk? I'd like to extend this further in follow-up patches: 1) fold always-false/always-true comparisons e.g. short x; (float)x > INT16_MAX; // Always false 2) get rid of cast in comparisons with zero regardless of typeof(lhs) when -fno-trapping-math: (float_or_double)lhs CMP 0 -Y pr57371-1.patch 2017-07-02 Yury Gribov PR tree-optimization/57371 * match.pd: New pattern. * testsuite/gcc.dg/pr57371-1.c: New test. * testsuite/gcc.dg/pr57371-2.c: New test. diff -rupN gcc/gcc/match.pd gcc-57371/gcc/match.pd --- gcc/gcc/match.pd2017-06-29 21:14:57.0 +0200 +++ gcc-57371/gcc/match.pd 2017-07-01 09:08:04.0 +0200 @@ -2802,7 +2802,35 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (simplify (cmp (sq @0) (sq @1)) (if (! HONOR_NANS (@0)) - (cmp @0 @1)) + (cmp @0 @1) + + /* Get rid of float cast in + (float_type)N CMP M +if N and M are within the range explicitly representable +by float type. + +TODO: fold always true/false comparisons if M is outside valid range. */ + (simplify + (cmp (float @0) REAL_CST@1) + (if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (@1))) + (with +{ + tree itype = TREE_TYPE (@0); + + const real_format *fmt = REAL_MODE_FORMAT (TYPE_MODE (TREE_TYPE (@1))); + + const REAL_VALUE_TYPE *rhs = TREE_REAL_CST_PTR (@1); + bool not_rhs_int_p = false; + wide_int rhs_int = real_to_integer (rhs, ¬_rhs_int_p, TYPE_PRECISION (itype)); +} +(if (!not_rhs_int_p + && !(TYPE_UNSIGNED (itype) && real_isneg (rhs)) + && wi::ge_p (rhs_int, wi::min_value (itype), TYPE_SIGN (itype)) + && wi::le_p (rhs_int, wi::max_value (itype), TYPE_SIGN (itype)) + && TYPE_PRECISION (itype) <= significand_size (fmt)) + (cmp @0 { wide_int_to_tree (itype, rhs_int); }) + +) Seems like a nit, but instead of "not_rhs_int_p" use "fail" or something like that. That makes it easier to mentally parse the conditional which uses the result. What happens if @0 is a floating point type? Based on the variable name "itype" and passing TYPE_PRECISION (itype) to real_to_integer, it seems like you're expecting @0 to be an integer. If so, you should verify that it really is an integer type. Seems like a good thing to verify with tests as well. @0 is the argument of a FLOAT_EXPR. verify_gimple_assign_unary guarantees that it is INTEGRAL_TYPE_P (or VECTOR_INTEGER_TYPE_P but then the result would have to be VECTOR_FLOAT_TYPE_P, and since it gets compared to REAL_CST... the test SCALAR_FLOAT_TYPE_P is actually redundant). -- Marc Glisse
Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons
On Mon, Jul 3, 2017 at 4:28 PM, Jeff Law wrote: > On 07/03/2017 08:52 AM, Joseph Myers wrote: >> I'd expect much more thorough testcases here, both for cases that get >> optimized and cases that don't. You're only testing comparisons with >> zero. There should be comparisons with other values, both integer and >> noninteger, both within the range for which optimizing would be valid and >> outside it, both inside the range of the integer type and outside it. >> (To the extent that you don't optimize some cases that would be valid to >> optimize as discussed in that PR, XFAILed tests, or deferring adding >> tests, would be reasonable. But each case identified in that PR as not >> valid to optimize, or only valid to optimize with -fno-trapping-math, >> should have corresponding tests that it's not optimized.) >> >> Since SCALAR_FLOAT_TYPE_P includes decimal floating-point types, tests >> with those are desirable as well (in gcc.dg/dfp or c-c++-common/dfp, I >> suppose). >> > Agreed. I think with better testing this should be able to move forward > after the technical review. It's not terribly different conceptually > than the code in DOM/VRP, except that Yuri's changes work on floating > point types. > > I'm pretty sure DOM's bits could be replaced with a suitable match.pd > pattern (which IMHO would be a small improvement across multiple axis). > VRP would be more difficult as the VRP implementation depends on getting > the value range of the RHS of the conditional. Joseph, Jeff, Thanks a lot for your comments. I'll work on updated version and post it (hopefully) soon. -Y
Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons
On Mon, Jul 3, 2017 at 4:38 PM, Jeff Law wrote: > On 07/02/2017 11:03 AM, Yuri Gribov wrote: >> Hi all, >> >> This is initial patch for >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 . Per Joseph's >> suggestion it optimizes >> (float)lhs CMP rhs >> (double)lhs CMP rhs >> to >> lhs CMP (typeof(x))rhs >> whenever typeof(x) can be precisely represented by floating-point type >> (e.g. short by float or int by double) and rhs can be precisely >> represented by typeof(x). >> >> Bootstrapped/regtested on x64. Ok for trunk? >> >> I'd like to extend this further in follow-up patches: >> 1) fold always-false/always-true comparisons e.g. >> short x; >> (float)x > INT16_MAX; // Always false >> 2) get rid of cast in comparisons with zero regardless of typeof(lhs) >> when -fno-trapping-math: >> (float_or_double)lhs CMP 0 >> >> -Y >> >> >> pr57371-1.patch >> >> >> 2017-07-02 Yury Gribov >> >> PR tree-optimization/57371 >> * match.pd: New pattern. >> * testsuite/gcc.dg/pr57371-1.c: New test. >> * testsuite/gcc.dg/pr57371-2.c: New test. >> >> diff -rupN gcc/gcc/match.pd gcc-57371/gcc/match.pd >> --- gcc/gcc/match.pd 2017-06-29 21:14:57.0 +0200 >> +++ gcc-57371/gcc/match.pd2017-07-01 09:08:04.0 +0200 >> @@ -2802,7 +2802,35 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) >> (simplify >> (cmp (sq @0) (sq @1)) >>(if (! HONOR_NANS (@0)) >> - (cmp @0 @1)) >> + (cmp @0 @1) >> + >> + /* Get rid of float cast in >> + (float_type)N CMP M >> +if N and M are within the range explicitly representable >> +by float type. >> + >> +TODO: fold always true/false comparisons if M is outside valid range. >> */ >> + (simplify >> + (cmp (float @0) REAL_CST@1) >> + (if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (@1))) >> + (with >> +{ >> + tree itype = TREE_TYPE (@0); >> + >> + const real_format *fmt = REAL_MODE_FORMAT (TYPE_MODE (TREE_TYPE >> (@1))); >> + >> + const REAL_VALUE_TYPE *rhs = TREE_REAL_CST_PTR (@1); >> + bool not_rhs_int_p = false; >> + wide_int rhs_int = real_to_integer (rhs, ¬_rhs_int_p, >> TYPE_PRECISION (itype)); >> +} >> +(if (!not_rhs_int_p >> + && !(TYPE_UNSIGNED (itype) && real_isneg (rhs)) >> + && wi::ge_p (rhs_int, wi::min_value (itype), TYPE_SIGN (itype)) >> + && wi::le_p (rhs_int, wi::max_value (itype), TYPE_SIGN (itype)) >> + && TYPE_PRECISION (itype) <= significand_size (fmt)) >> + (cmp @0 { wide_int_to_tree (itype, rhs_int); }) >> + >> +) > Seems like a nit, but instead of "not_rhs_int_p" use "fail" or something > like that. That makes it easier to mentally parse the conditional which > uses the result. Actually it's even worse than that, it should actually be overflow_p and for not_rhs_int_p I need to use other APIs. > What happens if @0 is a floating point type? Based on the variable name > "itype" and passing TYPE_PRECISION (itype) to real_to_integer, it seems > like you're expecting @0 to be an integer. If so, you should verify > that it really is an integer type. Seems like a good thing to verify > with tests as well. Right. -Y
Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons
On 07/02/2017 11:03 AM, Yuri Gribov wrote: > Hi all, > > This is initial patch for > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 . Per Joseph's > suggestion it optimizes > (float)lhs CMP rhs > (double)lhs CMP rhs > to > lhs CMP (typeof(x))rhs > whenever typeof(x) can be precisely represented by floating-point type > (e.g. short by float or int by double) and rhs can be precisely > represented by typeof(x). > > Bootstrapped/regtested on x64. Ok for trunk? > > I'd like to extend this further in follow-up patches: > 1) fold always-false/always-true comparisons e.g. > short x; > (float)x > INT16_MAX; // Always false > 2) get rid of cast in comparisons with zero regardless of typeof(lhs) > when -fno-trapping-math: > (float_or_double)lhs CMP 0 > > -Y > > > pr57371-1.patch > > > 2017-07-02 Yury Gribov > > PR tree-optimization/57371 > * match.pd: New pattern. > * testsuite/gcc.dg/pr57371-1.c: New test. > * testsuite/gcc.dg/pr57371-2.c: New test. > > diff -rupN gcc/gcc/match.pd gcc-57371/gcc/match.pd > --- gcc/gcc/match.pd 2017-06-29 21:14:57.0 +0200 > +++ gcc-57371/gcc/match.pd2017-07-01 09:08:04.0 +0200 > @@ -2802,7 +2802,35 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > (simplify > (cmp (sq @0) (sq @1)) >(if (! HONOR_NANS (@0)) > - (cmp @0 @1)) > + (cmp @0 @1) > + > + /* Get rid of float cast in > + (float_type)N CMP M > +if N and M are within the range explicitly representable > +by float type. > + > +TODO: fold always true/false comparisons if M is outside valid range. */ > + (simplify > + (cmp (float @0) REAL_CST@1) > + (if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (@1))) > + (with > +{ > + tree itype = TREE_TYPE (@0); > + > + const real_format *fmt = REAL_MODE_FORMAT (TYPE_MODE (TREE_TYPE (@1))); > + > + const REAL_VALUE_TYPE *rhs = TREE_REAL_CST_PTR (@1); > + bool not_rhs_int_p = false; > + wide_int rhs_int = real_to_integer (rhs, ¬_rhs_int_p, > TYPE_PRECISION (itype)); > +} > +(if (!not_rhs_int_p > + && !(TYPE_UNSIGNED (itype) && real_isneg (rhs)) > + && wi::ge_p (rhs_int, wi::min_value (itype), TYPE_SIGN (itype)) > + && wi::le_p (rhs_int, wi::max_value (itype), TYPE_SIGN (itype)) > + && TYPE_PRECISION (itype) <= significand_size (fmt)) > + (cmp @0 { wide_int_to_tree (itype, rhs_int); }) > + > +) Seems like a nit, but instead of "not_rhs_int_p" use "fail" or something like that. That makes it easier to mentally parse the conditional which uses the result. What happens if @0 is a floating point type? Based on the variable name "itype" and passing TYPE_PRECISION (itype) to real_to_integer, it seems like you're expecting @0 to be an integer. If so, you should verify that it really is an integer type. Seems like a good thing to verify with tests as well. Jeff
Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons
On 07/03/2017 08:52 AM, Joseph Myers wrote: > I'd expect much more thorough testcases here, both for cases that get > optimized and cases that don't. You're only testing comparisons with > zero. There should be comparisons with other values, both integer and > noninteger, both within the range for which optimizing would be valid and > outside it, both inside the range of the integer type and outside it. > (To the extent that you don't optimize some cases that would be valid to > optimize as discussed in that PR, XFAILed tests, or deferring adding > tests, would be reasonable. But each case identified in that PR as not > valid to optimize, or only valid to optimize with -fno-trapping-math, > should have corresponding tests that it's not optimized.) > > Since SCALAR_FLOAT_TYPE_P includes decimal floating-point types, tests > with those are desirable as well (in gcc.dg/dfp or c-c++-common/dfp, I > suppose). > Agreed. I think with better testing this should be able to move forward after the technical review. It's not terribly different conceptually than the code in DOM/VRP, except that Yuri's changes work on floating point types. I'm pretty sure DOM's bits could be replaced with a suitable match.pd pattern (which IMHO would be a small improvement across multiple axis). VRP would be more difficult as the VRP implementation depends on getting the value range of the RHS of the conditional. Jeff
Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons
I'd expect much more thorough testcases here, both for cases that get optimized and cases that don't. You're only testing comparisons with zero. There should be comparisons with other values, both integer and noninteger, both within the range for which optimizing would be valid and outside it, both inside the range of the integer type and outside it. (To the extent that you don't optimize some cases that would be valid to optimize as discussed in that PR, XFAILed tests, or deferring adding tests, would be reasonable. But each case identified in that PR as not valid to optimize, or only valid to optimize with -fno-trapping-math, should have corresponding tests that it's not optimized.) Since SCALAR_FLOAT_TYPE_P includes decimal floating-point types, tests with those are desirable as well (in gcc.dg/dfp or c-c++-common/dfp, I suppose). -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons
On Sun, Jul 2, 2017 at 6:03 PM, Yuri Gribov wrote: > Hi all, > > This is initial patch for > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 . Per Joseph's > suggestion it optimizes > (float)lhs CMP rhs > (double)lhs CMP rhs > to > lhs CMP (typeof(x))rhs > whenever typeof(x) can be precisely represented by floating-point type > (e.g. short by float or int by double) and rhs can be precisely > represented by typeof(x). > > Bootstrapped/regtested on x64. Ok for trunk? Seems my understanding of real_to_integer was too naive. I'll fix the patch and send updated variant. -Y
Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons
On Sun, 2 Jul 2017, Yuri Gribov wrote: On Sun, Jul 2, 2017 at 6:32 PM, Marc Glisse wrote: On Sun, 2 Jul 2017, Yuri Gribov wrote: This is initial patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 . Thanks. I have had this unfinished, probably wrong patch on my hard drive for 4 years, I doubt there is much you can extract from it, but just in case... Thanks, this will indeed help! I only don't like !uns here: + bool uns = TYPE_UNSIGNED (itype); + bool fits = mantissa >= prec - !uns; as I believe for INT?_MIN we need 'prec' number of bits. INT_MIN is (minus) a power of 2, so 1 bit of mantissa is sufficient to represent it exactly (as long as the exponent also fits). At least for an IEEE754 representation using base 2, but I expect non-IEEE representations of floats are similar enough. -- Marc Glisse
Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons
On Sun, Jul 2, 2017 at 6:32 PM, Marc Glisse wrote: > On Sun, 2 Jul 2017, Yuri Gribov wrote: > >> This is initial patch for >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 . > > Thanks. I have had this unfinished, probably wrong patch on my hard drive > for 4 years, I doubt there is much you can extract from it, but just in > case... Thanks, this will indeed help! I only don't like !uns here: + bool uns = TYPE_UNSIGNED (itype); + bool fits = mantissa >= prec - !uns; as I believe for INT?_MIN we need 'prec' number of bits. -Y
Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons
On Sun, 2 Jul 2017, Yuri Gribov wrote: This is initial patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 . Thanks. I have had this unfinished, probably wrong patch on my hard drive for 4 years, I doubt there is much you can extract from it, but just in case... -- Marc GlisseIndex: gcc/fold-const.c === --- gcc/fold-const.c(revision 209514) +++ gcc/fold-const.c(working copy) @@ -6389,21 +6389,21 @@ fold_inf_compare (location_t loc, enum t /* x > +Inf is always false, if with ignore sNANs. */ if (HONOR_SNANS (mode)) return NULL_TREE; return omit_one_operand_loc (loc, type, integer_zero_node, arg0); case LE_EXPR: /* x <= +Inf is always true, if we don't case about NaNs. */ if (! HONOR_NANS (mode)) return omit_one_operand_loc (loc, type, integer_one_node, arg0); - /* x <= +Inf is the same as x == x, i.e. isfinite(x). */ + /* x <= +Inf is the same as x == x, i.e. !isnan(x). */ arg0 = save_expr (arg0); return fold_build2_loc (loc, EQ_EXPR, type, arg0, arg0); case EQ_EXPR: case GE_EXPR: /* x == +Inf and x >= +Inf are always equal to x > DBL_MAX. */ real_maxval (&max, neg, mode); return fold_build2_loc (loc, neg ? LT_EXPR : GT_EXPR, type, arg0, build_real (TREE_TYPE (arg0), max)); @@ -9389,70 +9389,121 @@ fold_comparison (location_t loc, enum tr tem = maybe_canonicalize_comparison (loc, code, type, arg0, arg1); if (tem) return tem; if (FLOAT_TYPE_P (TREE_TYPE (arg0))) { tree targ0 = strip_float_extensions (arg0); tree targ1 = strip_float_extensions (arg1); tree newtype = TREE_TYPE (targ0); + tree ftype = TREE_TYPE (arg0); if (TYPE_PRECISION (TREE_TYPE (targ1)) > TYPE_PRECISION (newtype)) newtype = TREE_TYPE (targ1); /* Fold (double)float1 CMP (double)float2 into float1 CMP float2. */ - if (TYPE_PRECISION (newtype) < TYPE_PRECISION (TREE_TYPE (arg0))) + if (TYPE_PRECISION (newtype) < TYPE_PRECISION (ftype)) return fold_build2_loc (loc, code, type, fold_convert_loc (loc, newtype, targ0), fold_convert_loc (loc, newtype, targ1)); /* (-a) CMP (-b) -> b CMP a */ if (TREE_CODE (arg0) == NEGATE_EXPR && TREE_CODE (arg1) == NEGATE_EXPR) return fold_build2_loc (loc, code, type, TREE_OPERAND (arg1, 0), TREE_OPERAND (arg0, 0)); if (TREE_CODE (arg1) == REAL_CST) { REAL_VALUE_TYPE cst; cst = TREE_REAL_CST (arg1); /* (-a) CMP CST -> a swap(CMP) (-CST) */ if (TREE_CODE (arg0) == NEGATE_EXPR) return fold_build2_loc (loc, swap_tree_comparison (code), type, TREE_OPERAND (arg0, 0), - build_real (TREE_TYPE (arg1), + build_real (ftype, real_value_negate (&cst))); /* IEEE doesn't distinguish +0 and -0 in comparisons. */ /* a CMP (-0) -> a CMP 0 */ if (REAL_VALUE_MINUS_ZERO (cst)) return fold_build2_loc (loc, code, type, arg0, - build_real (TREE_TYPE (arg1), dconst0)); + build_real (ftype, dconst0)); /* x != NaN is always true, other ops are always false. */ if (REAL_VALUE_ISNAN (cst) - && ! HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg1 + && ! HONOR_SNANS (TYPE_MODE (ftype))) { tem = (code == NE_EXPR) ? integer_one_node : integer_zero_node; return omit_one_operand_loc (loc, type, tem, arg0); } /* Fold comparisons against infinity. */ if (REAL_VALUE_ISINF (cst) - && MODE_HAS_INFINITIES (TYPE_MODE (TREE_TYPE (arg1 + && MODE_HAS_INFINITIES (TYPE_MODE (ftype))) { tem = fold_inf_compare (loc, code, type, arg0, arg1); if (tem != NULL_TREE) return tem; } + + /* (double)i CMP cst is often just i CMP cst'. +See PR 57371 for a detailed analysis and other ideas. */ + if (TREE_CODE (arg0) == FLOAT_EXPR) + { + tree inner = TREE_OPERAND (arg0, 0); + tree itype = TREE_TYPE (inner); + unsigned mantissa = significand_size (TYPE_MODE (ftype)); + unsigned prec = element_precision (itype); + bool uns = TYPE_UNSIGNED (itype); + bool fits = mantissa >= prec - !uns; + /* If ftype cannot represent exactly all values of itype, +we may have an inexact exception. If the conversion from +itype to ftype may overflow (unsigned __int128 to float), +
[PATCH][PR 57371] Remove useless floating point casts in comparisons
Hi all, This is initial patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 . Per Joseph's suggestion it optimizes (float)lhs CMP rhs (double)lhs CMP rhs to lhs CMP (typeof(x))rhs whenever typeof(x) can be precisely represented by floating-point type (e.g. short by float or int by double) and rhs can be precisely represented by typeof(x). Bootstrapped/regtested on x64. Ok for trunk? I'd like to extend this further in follow-up patches: 1) fold always-false/always-true comparisons e.g. short x; (float)x > INT16_MAX; // Always false 2) get rid of cast in comparisons with zero regardless of typeof(lhs) when -fno-trapping-math: (float_or_double)lhs CMP 0 -Y pr57371-1.patch Description: Binary data