Re: [PATCH v7] rtl: builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]

[Jeff Law <jeffreyalaw@gmail.com>]

On 11/24/2021 4:48 PM, Raoni Fassina Firmino via Gcc-patches wrote:
> Changes since v6[6] and v5[5]:
>    - Based this version on the v5 one.
>    - Reworked all builtins back to the way they are in v5 and added the
>      following changes:
>      + Added a test to target libc, only expanding with glibc as the
>        target libc.
>      + Updated all three expanders header comment to reflect the added
>        behavior (fegetround got a full header as it had none).
>      + Added extra documentation for the builtins on doc/extend.texi,
>        similar to v6 version, but only the introductory paragraph,
>        without a dedicated entry for each, since now they behavior and
>        signature match the C99 ones.
>    - Changed the description for the return operand in the RTL template
>      of the fegetround expander.  Using "(set )", the same way as
>      rs6000_mffsl expander (this change was taken from v6).
>    - Updated the commit message mentioning the target libc restriction
>      and updated changelog.
>
> Tested on top of master (9bf69a8558638ce0cdd69e83a68776deb9b8e053)
> on the following plataforms with no regression:
>    - powerpc64le-linux-gnu (Power 9)
>    - powerpc64le-linux-gnu (Power 8)
>    - powerpc64-linux-gnu (Power 9, with 32 and 64 bits tests)
>
> Also made a visual test comparing the generated assembly of a test
> program built against glibc and musl (with -mmusl and with musl-gcc).
>
> Documentation changes tested on x86_64-redhat-linux.
>
> Well, turns out v6 was kind of a misstep[7].  But turns out the
> solution was in my face the whole time and Joseph was kind enough to
> spell it out to me.  I should have known, one can check for the target
> libc at runtime. It is a really simple addition to each expander, only
> expanding for the libcs the expander know the FE_* and can handle it.
> As Joseph mentioned on his review, with that the expander don't have
> to always expand and everything is fine.
>
> As I mentioned[8], musl and uclibc both uses the same values as glibc,
> I could add then enabling the expanders for them, not sure about it.
>
> I don't know if I should add something to the documentation, more
> precisely on section "6.59 Other Built-in Functions Provided by GCC"
> in doc/extend.text. Like I mentioned in v6 but I don't know if I'm
> doing it right, especially changing such a front facing documentation,
> but here it is.
>
> I'm repeating the "changelog" from past versions here for convenience:
>
> Changes since v5[5]:
>    - Reworked all builtins to accept the FE_* macros as parameters and
>      so be agnostic to libc implementations.  Largely based of
>      fpclassify.  To that end, there is some new files changed:
>      + Change the argument list for the builtins declarations in
>        builtins.def
>      + Added new types in builtin-types.def to use in the buitins
>        declarations.
>      + Added extra documentation for the builtins on doc/extend.texi,
>        similar to fpclassify.
>    - Updated doc/md.texi documentation with the new optab behaviors.
>    - Updated comments to the expanders and expand handlers to try to
>      explain whats is going on.
>    - Changed the description for the return operand in the RTL template
>      of the fegetround expander.  Using "(set )", the same way as
>      rs6000_mffsl expander.
>    - Updated testcases with helper macros with the new argument list.
>
> Changes since v4[4]:
>    - Fixed more spelling and code style.
>    - Add more clarification on  comments for feraiseexcept and
>      feclearexcept expands;
>
> Changes since v3[3]:
>    - Fixed fegetround bug on powerpc64 (big endian) that Segher
>      spotted;
>
> Changes since v2[2]:
>    - Added documentation for the new optabs;
>    - Remove use of non portable __builtin_clz;
>    - Changed feclearexcept and feraiseexcept to accept all 4 valid
>      flags at the same time and added more test for that case;
>    - Extended feclearexcept and feraiseexcept testcases to match
>      accepting multiple flags;
>    - Fixed builtin-feclearexcept-feraiseexcept-2.c testcase comparison
>      after feclearexcept tests;
>    - Updated commit message to reflect change in feclearexcept and
>      feraiseexcept from the glibc counterpart;
>    - Fixed English spelling and typos;
>    - Fixed code-style;
>    - Changed subject line tag to make clear it is not just rs6000 code.
>
> Changes since v1[1]:
>    - Fixed English spelling;
>    - Fixed code-style;
>    - Changed match operand predicate in feclearexcept and feraiseexcept;
>    - Changed testcase options;
>    - Minor changes in test code to be C90 compatible;
>    - Other minor changes suggested by Segher;
>    - Changed subject line tag (not sure if I tagged correctly or should
>      include optabs: also)
>
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552024.html
> [2] https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553297.html
> [3] https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557109.html
> [4] https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557349.html
> [5] https://gcc.gnu.org/pipermail/gcc-patches/2020-November/557984.html
> [6] https://gcc.gnu.org/pipermail/gcc-patches/2021-October/581837.html
> [7] https://gcc.gnu.org/pipermail/gcc-patches/2021-October/581929.html
> [8] https://gcc.gnu.org/pipermail/gcc-patches/2020-November/558070.html
>
> ---- 8< ----
>
> This optimizations were originally in glibc, but was removed
> and suggested that they were a good fit as gcc builtins[1].
>
> feclearexcept and feraiseexcept were extended (in comparison to the
> glibc version) to accept any combination of the accepted flags, not
> limited to just one flag bit at a time anymore.
>
> The builtin expanders needs knowledge of the target libc's FE_*
> values, so they are limited to expand only to suitable libcs.
>
> The associated bugreport: PR target/94193
>
> [1] https://sourceware.org/legacy-ml/libc-alpha/2020-03/msg00047.html
>      https://sourceware.org/legacy-ml/libc-alpha/2020-03/msg00080.html
>
> 2020-08-13  Raoni Fassina Firmino  <raoni@linux.ibm.com>
>
> gcc/ChangeLog:
>
>          * builtins.c (expand_builtin_fegetround): New function.
>          (expand_builtin_feclear_feraise_except): New function.
>          (expand_builtin): Add cases for BUILT_IN_FEGETROUND,
>          BUILT_IN_FECLEAREXCEPT and BUILT_IN_FERAISEEXCEPT
>          * config/rs6000/rs6000.md (fegetroundsi): New pattern.
>          (feclearexceptsi): New Pattern.
>          (feraiseexceptsi): New Pattern.
>          * doc/extend.texi: Add a new introductory paragraph about the
>          new builtins.
>          * doc/md.texi: (fegetround@var{m}): Document new optab.
>          (feclearexcept@var{m}): Document new optab.
>          (feraiseexcept@var{m}): Document new optab.
>          * optabs.def (fegetround_optab): New optab.
>          (feclearexcept_optab): New optab.
>          (feraiseexcept_optab): New optab.
>
> gcc/testsuite/ChangeLog:
>
>          * gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c: New test.
>          * gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c: New test.
>          * gcc.target/powerpc/builtin-fegetround.c: New test.
I think the generic parts are fine once Segher is happy with the rest of 
the bits.

jeff