Re: FW: [PING] Re: [Patch][GCC][middle-end] - Generate FRINTZ for (double)(int) under -ffast-math on aarch64

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Sep 24, 2021 at 2:59 PM Jirui Wu via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>
> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577846.html
>
> The patch is attached as text for ease of use. Is there anything that needs to change?
>
> Ok for master? If OK, can it be committed for me, I have no commit rights.

I'm still not sure about the correctness.  I suppose the
flag_fp_int_builtin_inexact && !flag_trapping_math is supposed to guard
against spurious inexact exceptions, shouldn't that be
!flag_fp_int_builtin_inexact || !flag_trapping_math instead?

The comment looks a bit redundant and we prefer sth like

/* (double)(int)x -> trunc (x) if the type of x matches the
expressions FP type.  */

Thanks,
Richard.

> Jirui Wu
>
> -----Original Message-----
> From: Jirui Wu
> Sent: Friday, September 10, 2021 10:14 AM
> To: Richard Biener <rguenther@suse.de>
> Cc: Richard Biener <richard.guenther@gmail.com>; Andrew Pinski <pinskia@gmail.com>; Richard Sandiford <Richard.Sandiford@arm.com>; ian@airs.com; gcc-patches@gcc.gnu.org; Joseph S. Myers <joseph@codesourcery.com>
> Subject: [PING] Re: [Patch][GCC][middle-end] - Generate FRINTZ for (double)(int) under -ffast-math on aarch64
>
> Hi,
>
> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577846.html
>
> Ok for master? If OK, can it be committed for me, I have no commit rights.
>
> Jirui Wu
> -----Original Message-----
> From: Jirui Wu
> Sent: Friday, September 3, 2021 12:39 PM
> To: 'Richard Biener' <rguenther@suse.de>
> Cc: Richard Biener <richard.guenther@gmail.com>; Andrew Pinski <pinskia@gmail.com>; Richard Sandiford <Richard.Sandiford@arm.com>; ian@airs.com; gcc-patches@gcc.gnu.org; Joseph S. Myers <joseph@codesourcery.com>
> Subject: RE: [Patch][GCC][middle-end] - Generate FRINTZ for (double)(int) under -ffast-math on aarch64
>
> Ping
>
> -----Original Message-----
> From: Jirui Wu
> Sent: Friday, August 20, 2021 4:28 PM
> To: Richard Biener <rguenther@suse.de>
> Cc: Richard Biener <richard.guenther@gmail.com>; Andrew Pinski <pinskia@gmail.com>; Richard Sandiford <Richard.Sandiford@arm.com>; ian@airs.com; gcc-patches@gcc.gnu.org; Joseph S. Myers <joseph@codesourcery.com>
> Subject: RE: [Patch][GCC][middle-end] - Generate FRINTZ for (double)(int) under -ffast-math on aarch64
>
> > -----Original Message-----
> > From: Richard Biener <rguenther@suse.de>
> > Sent: Friday, August 20, 2021 8:15 AM
> > To: Jirui Wu <Jirui.Wu@arm.com>
> > Cc: Richard Biener <richard.guenther@gmail.com>; Andrew Pinski
> > <pinskia@gmail.com>; Richard Sandiford <Richard.Sandiford@arm.com>;
> > ian@airs.com; gcc-patches@gcc.gnu.org; Joseph S. Myers
> > <joseph@codesourcery.com>
> > Subject: RE: [Patch][GCC][middle-end] - Generate FRINTZ for
> > (double)(int) under -ffast-math on aarch64
> >
> > On Thu, 19 Aug 2021, Jirui Wu wrote:
> >
> > > Hi all,
> > >
> > > This patch generates FRINTZ instruction to optimize type casts.
> > >
> > > The changes in this patch covers:
> > > * Generate FRINTZ for (double)(int) casts.
> > > * Add new test cases.
> > >
> > > The intermediate type is not checked according to the C99 spec.
> > > Overflow of the integral part when casting floats to integers causes
> > undefined behavior.
> > > As a result, optimization to trunc() is not invalid.
> > > I've confirmed that Boolean type does not match the matching condition.
> > >
> > > Regtested on aarch64-none-linux-gnu and no issues.
> > >
> > > Ok for master? If OK can it be committed for me, I have no commit rights.
> >
> > +/* Detected a fix_trunc cast inside a float type cast,
> > +   use IFN_TRUNC to optimize.  */
> > +#if GIMPLE
> > +(simplify
> > +  (float (fix_trunc @0))
> > +  (if (direct_internal_fn_supported_p (IFN_TRUNC, type,
> > +                                      OPTIMIZE_FOR_BOTH)
> > +       && flag_unsafe_math_optimizations
> > +       && type == TREE_TYPE (@0))
> >
> > types_match (type, TREE_TYPE (@0))
> >
> > please.  Please perform cheap tests first (the flag test).
> >
> > +     (IFN_TRUNC @0)))
> > +#endif
> >
> > why only for GIMPLE?  I'm not sure flag_unsafe_math_optimizations is a
> > good test here.  If you say we can use undefined behavior of any
> > overflow of the fix_trunc operation what do we guard here?
> > If it's Inf/NaN input then flag_finite_math_only would be more
> > appropriate, if it's behavior for -0. (I suppose trunc (-0.0) == -0.0
> > and thus "wrong") then a && !HONOR_SIGNED_ZEROS (type) is missing
> > instead.  If it's setting of FENV state and possibly trapping on
> > overflow (but it's undefined?!) then flag_trapping_math covers the
> > latter but we don't have any flag for eliding FENV state affecting
> > transforms, so there the kitchen-sink flag_unsafe_math_optimizations might apply.
> >
> > So - which is it?
> >
> This change is only for GIMPLE because we can't test for the optab support without being in GIMPLE. direct_internal_fn_supported_p is defined only for GIMPLE.
>
> IFN_TRUNC's documentation mentions nothing for zero, NaNs/inf inputs.
> So I think the correct guard is just flag_fp_int_builtin_inexact.
> !flag_trapping_math because the operation can only still raise inexacts.
>
> The new pattern is moved next to the place you mentioned.
>
> Ok for master? If OK can it be committed for me, I have no commit rights.
>
> Thanks,
> Jirui
> > Note there's also the pattern
> >
> > /* Handle cases of two conversions in a row.  */ (for ocvt (convert
> > float
> > fix_trunc)  (for icvt (convert float)
> >   (simplify
> >    (ocvt (icvt@1 @0))
> >    (with
> >     {
> > ...
> >
> > which is related so please put the new pattern next to that (the set
> > of conversions handled there does not include (float (fix_trunc @0)))
> >
> > Thanks,
> > Richard.
> >
> > > Thanks,
> > > Jirui
> > >
> > > gcc/ChangeLog:
> > >
> > >         * match.pd: Generate IFN_TRUNC.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > >         * gcc.target/aarch64/merge_trunc1.c: New test.
> > >
> > > > -----Original Message-----
> > > > From: Richard Biener <richard.guenther@gmail.com>
> > > > Sent: Tuesday, August 17, 2021 9:13 AM
> > > > To: Andrew Pinski <pinskia@gmail.com>
> > > > Cc: Jirui Wu <Jirui.Wu@arm.com>; Richard Sandiford
> > > > <Richard.Sandiford@arm.com>; ian@airs.com;
> > > > gcc-patches@gcc.gnu.org; rguenther@suse.de
> > > > Subject: Re: [Patch][GCC][middle-end] - Generate FRINTZ for
> > > > (double)(int) under -ffast-math on aarch64
> > > >
> > > > On Mon, Aug 16, 2021 at 8:48 PM Andrew Pinski via Gcc-patches
> > > > <gcc- patches@gcc.gnu.org> wrote:
> > > > >
> > > > > On Mon, Aug 16, 2021 at 9:15 AM Jirui Wu via Gcc-patches
> > > > > <gcc-patches@gcc.gnu.org> wrote:
> > > > > >
> > > > > > Hi all,
> > > > > >
> > > > > > This patch generates FRINTZ instruction to optimize type casts.
> > > > > >
> > > > > > The changes in this patch covers:
> > > > > > * Opimization of a FIX_TRUNC_EXPR cast inside a FLOAT_EXPR
> > > > > > using
> > > > IFN_TRUNC.
> > > > > > * Change of corresponding test cases.
> > > > > >
> > > > > > Regtested on aarch64-none-linux-gnu and no issues.
> > > > > >
> > > > > > Ok for master? If OK can it be committed for me, I have no
> > > > > > commit
> > rights.
> > > > >
> > > > > Is there a reason why you are doing the transformation manually
> > > > > inside forwprop rather than handling it inside match.pd?
> > > > > Also can't this only be done for -ffast-math case?
> > > >
> > > > You definitely have to look at the intermediate type - that could
> > > > be a uint8_t or even a boolean type.  So unless the intermediate
> > > > type can represent all float values optimizing to trunc() is invalid.
> > > > Also if you emit IFN_TRUNC you have to make sure there's target
> > > > support - we don't emit calls to a library
> > > > trunc() from an internal function call (and we wouldn't want to
> > > > optimize it that way).
> > > >
> > > > Richard.
> > > >
> > > > >
> > > > > Thanks,
> > > > > Andrew Pinski
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Jirui
> > > > > >
> > > > > > gcc/ChangeLog:
> > > > > >
> > > > > >         * tree-ssa-forwprop.c (pass_forwprop::execute):
> > > > > > Optimize with
> > frintz.
> > > > > >
> > > > > >
> > > > > > gcc/testsuite/ChangeLog:
> > > > > >
> > > > > >         * gcc.target/aarch64/fix_trunc1.c: Update to new expectation.
> > >
> >
> > --
> > Richard Biener <rguenther@suse.de>
> > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409
> > Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)