This is the mail archive of the
`gcc-patches@gcc.gnu.org`
mailing list for the GCC project.

Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|

Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |

Other format: | [Raw text] |

*From*: Joseph Myers <joseph at codesourcery dot com>*To*: Tejas Joshi <tejasjoshi9673 at gmail dot com>*Cc*: <gcc-patches at gcc dot gnu dot org>, Martin Jambor <mjambor at suse dot cz>, <hubicka at ucw dot cz>*Date*: Wed, 21 Aug 2019 16:55:35 +0000*Subject*: Re: [PATCH] builtin fadd variants implementation*Ironport-sdr*: G6vRZbo4Q9qYIYlEyWChYiQZdhj8vW5sEF7IvHZYanWq2ocy+UOiS+tFwVwpRv+jP0ER+86f/f PXwrYIDsPqWh9OtAbCfSyBvXLt1772fONDYCybEymaLb5SlkKMLjInE/SrILVIlm1+sjugx+79 APZYUVfusS3IshVM8T9bnBWu+DB0xRghF+QIcoSGmLDQEh6ee7eJY+vPoBuMlDZaVnYESVIu8Q GY4vwLZcUv16vEjcDfGsGmqPZxPOL9mhihkpsGbLCFNghNOUXi78U4wUhHwLE5R6b8UOGT7k8f nto=*Ironport-sdr*: f7mdlzrcHj4GTOwQHgQv3IJr1DyvlcOX/coR8TvcPmBASZJ0J67Jox/pE1uI5G0wqmY5lVLeO1 aCIaHwrYq7clBYd4vwW4yxIwbc8VTuM6gMLyj8BcXPwxm/+K8l3HNfReCSPnUBzietclS1KwTT MgZXsrutnPbg1XMEp0fsiPQsbddxiwAQmyuWFi0SWZvvphGGF/k2CBkheDqOk61qvOYGvkUXs8 SGEYI5KR5K8ermH+ENZD5Br8J6p2LFbvzl1SEKhAcILqX0Wuq7H1LWjU0KszUXEqN67epnAlZC 7os=*References*: <CACMrGjA_V1D9UJ=Ldy+HfkCub3Tw6rdFAVr0nuxvtu6aLGZgYA@mail.gmail.com>

On Thu, 8 Aug 2019, Tejas Joshi wrote: > +/* Try to evaluate: > + > + *RESULT = fadd (*ARG0, *ARG1) > + > + in format FORMAT. Return true on success. */ > + > +static bool > +fold_const_fadd (real_value *result, const real_value *arg0, > + const real_value *arg1, const real_format *format) > +{ > + if (REAL_VALUE_ISSIGNALING_NAN (*arg0) > + || REAL_VALUE_ISSIGNALING_NAN (*arg1)) > + return false; > + > + if (real_fadd (result, format, arg0, arg1) > + && !flag_errno_math) > + return false; I think that rather than having a real_fadd function, you should just call real_arithmetic with a PLUS_EXPR argument and then move the overflow etc. checks out to here. That way, rather than having a fold_const_fadd function and then similar functions for fsub / fmul / fdiv, you can have one function that covers all four binary narrowing operations, which all have very similar logic, with an argument that is PLUS_EXPR etc. passed to the fold_const_narrow_binary (or whatever you call it) function. > + if (!exact_real_truncate (format, result) > + && !(flag_rounding_math && flag_trapping_math)) > + return false; I don't think the logic is right here. What you want is: don't fold for inexact results if flag_rounding_math || flag_trapping_math. I.e.: if (!exact_real_truncate (format, result) && (flag_rounding_math || flag_trapping_math)) > +/* Perform addition of X and Y and place the result in R to narrowed > + down type. Return TRUE if result involves overflow/underflow. */ > + > +bool > +real_fadd (REAL_VALUE_TYPE *r, format_helper fmt, > + const REAL_VALUE_TYPE *x, const REAL_VALUE_TYPE *y) > +{ > + do_add (r, x, y, 0); > + > + /* Overflow/underflow check. */ > + if (REAL_EXP (r) > fmt->emax) > + { > + get_inf (r, r->sign); > + return true; > + } This isn't the right way to check for overflow. You can have a result that's within the exponent range of the narrower format before rounding, but overflows after the rounding is done. The right way to check for overflow (given that the result is inexact, and !flag_rounding_math && !flag_trapping_math && flag_errno_math so that inexact results are OK but not if they overflow) would be to check after rounding whether the result is Inf when the arguments were finite. (When you get only fdiv, that also becomes relevant for exact cases; fdiv (1.0, 0.0) produces an exact infinity but can't be folded if flag_errno_math || flag_trapping_math.) As well as overflow you also need to consider the case of fadd (Inf, -Inf) which would set errno to EDOM and raise "invalid" so, although exact, is also not allowed to be folded if flag_errno_math || flag_trapping_math. The generic logic there is: if the arguments are not NaN but the result is NaN, you can't fold if flag_errno_math || flag_trapping_math (but flag_rounding_math is irrelevant in that case). > diff --git a/gcc/testsuite/gcc.dg/builtin-fadd-1.c b/gcc/testsuite/gcc.dg/builtin-fadd-1.c > new file mode 100644 > index 00000000000..958d5a019d7 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/builtin-fadd-1.c > @@ -0,0 +1,31 @@ > +/* { dg-do run } */ > +/* { dg-options "-O2" } */ > + > +#define TEST(FN, VAL1, VAL2, RESULT) \ > + if (__builtin_##FN (VAL1, VAL2) != RESULT) __builtin_abort (); The tests ought to be tests of what gets optimized away (so causing link failures, as in the roundeven tests, if the calls don't get optimized or get optimized incorrectly). The tests should include cases that demonstrate there is only a single rounding involved: that there is no intermediate rounding to the wider type. (E.g. fadd (0x1.000001p0, FLT_MIN), as an example from the glibc tests: cases where an intermediate rounding produces a result half way between two values of the narrower type, but the exact value is such that the result of fadd should end up with last bit odd whereas double rounding would result in last bit even in such half-way cases.) > + TEST(fadd, __FLT_MAX__, __FLT_MAX__/2, 3*__FLT_MAX__/2); It would seem better to write the expected result here as __builtin_inff (). Then you should have some tests of what does *not* get optimized with given compiler options if possible. (Such a test might e.g. define a static fadd function locally that checks it gets called as expected, or else check the exceptions / errno if you rely on a suitable libm being available.) -- Joseph S. Myers joseph@codesourcery.com

**Follow-Ups**:**Re: [PATCH] builtin fadd variants implementation***From:*Tejas Joshi

**References**:**[PATCH] builtin fadd variants implementation***From:*Tejas Joshi

Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|

Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |