[EXTERNAL] Re: [PATCH] rs6000, restrict bfloat convert intrinsic to Power 10. Fix BU_P10V macro definitions.
Tue Aug 25 16:56:58 GMT 2020
On Mon, 2020-08-24 at 14:39 -0700, Carl Love wrote:
> On Wed, 2020-08-19 at 15:16 -0500, Segher Boessenkool wrote:
> > On Wed, Aug 19, 2020 at 02:19:12PM -0500, Peter Bergner wrote:
> > > On 8/14/20 7:42 PM, Segher Boessenkool wrote:
> > > > I think your current code is fine; I hadn't considered Bill's
> > > > upcoming
> > > > rewrite. It is more important to make that go smoother than to
> > > > fix some
> > > > aesthetics right now.
> > > >
> > > > Please check if the names for the builtins match the (future)
> > > > documentation, and then, approved for trunk. Thank you!
> > >
> > > This is also a bug in GCC 10, so this should be backported too.
> > >
> > > Segher, is this ok for Carl to backport to GCC 10 after it has sat
> > > on
> > > trunk for a few days?
> > Yes. Thanks guys.
> I attempted to do the backport however the patch doesn't even come
> close to applying. The names XVCVBF16SPN and XVCVSPBF16 are the only
> two builtin names that exist in GCC 10. The other issue is there is no
> Power 10 builtin macro definitions in GCC 10. So basically, I can fix
> the issue with XVCVBF16SPN and XVCVSPBF16 to be restricted to Power 10
> by adding the needed Power 10 macro definition.
> This is a whole new patch so I figure it needs to be reviewed to make
> sure we want to make this change to GCC 10. I did run the regression
> tests again using a Power 9 machine to verify it complies and there are
> no regression test failures.
I recommend a pure and clean description of what the patch is doing in a paragraph, separate from the history.
This patch corrects the built-in definitions for cvcvspbf16 and xvcvbf16spn to restrict their use to P10 and beyond in a way that
is consistent with the P8 and P9 builtin naming conventions.
This is a subset of changes made to trunk ...
> Please let me know if this is OK for the GCC 10 tree. Thanks.
> Carl Love
> [PATCH] rs6000, restrict bfloat convert intrinsic to Power 10. Back port to GCC 10.
> 2020-08-24 Carl Love <email@example.com>
> * config/rs6000/rs6000-builtin.def: (BU_P10V_VSX_1) New builtin macro expansion.
> (XVCVBF16SPN, XVCVSPBF16): Replace macro expansion BU_VSX_1 with BU_P10V_VSX_1.
OK as long as it's clear 'replace' means the define being used, versus the macro expansions themselves being replaced.
> * config/rs6000/rs6000-call.c: (VSX_BUILTIN_XVCVSPBF16, VSX_BUILTIN_XVCVBF16SPN):
> Replace with P10V_BUILTIN_XVCVSPBF16, P10V_BUILTIN_XVCVBF16SPN respectively.
s/Replace with/Rename to/ ?
> gcc/config/rs6000/rs6000-builtin.def | 14 ++++++++++++--
> gcc/config/rs6000/rs6000-call.c | 4 ++--
> 2 files changed, 14 insertions(+), 4 deletions(-)
> diff --git a/gcc/config/rs6000/rs6000-builtin.def b/gcc/config/rs6000/rs6000-builtin.def
> index 88f78cb0a15..512b7629a46 100644
> --- a/gcc/config/rs6000/rs6000-builtin.def
> +++ b/gcc/config/rs6000/rs6000-builtin.def
> @@ -1014,6 +1014,16 @@
> | RS6000_BTC_BINARY), \
> CODE_FOR_ ## ICODE) /* ICODE */
> +/* For builtins for power10 vector instructions that are encoded as altivec
> + instructions, use __builtin_altivec_ as the builtin name. */
That comment doesn't seem to apply to this change, update or drop ?
> +#define BU_P10V_VSX_1(ENUM, NAME, ATTR, ICODE)\
> + RS6000_BUILTIN_1 (P10_BUILTIN_ ## ENUM, /* ENUM */ \
> + "__builtin_vsx_" NAME, /* NAME */ \
> + RS6000_BTM_P10, /* MASK */ \
> + (RS6000_BTC_ ## ATTR /* ATTR */ \
> + | RS6000_BTC_UNARY), \
> + CODE_FOR_ ## ICODE) /* ICODE */
> @@ -2698,8 +2708,8 @@ BU_SPECIAL_X (RS6000_BUILTIN_CFSTRING, "__builtin_cfstring", RS6000_BTM_ALWAYS,
> /* POWER10 MMA builtins. */
> -BU_VSX_1 (XVCVBF16SPN, "xvcvbf16spn", MISC, vsx_xvcvbf16spn)
> -BU_VSX_1 (XVCVSPBF16, "xvcvspbf16", MISC, vsx_xvcvspbf16)
> +BU_P10V_VSX_1 (XVCVBF16SPN, "xvcvbf16spn", MISC, vsx_xvcvbf16spn)
> +BU_P10V_VSX_1 (XVCVSPBF16, "xvcvspbf16", MISC, vsx_xvcvspbf16)
> BU_MMA_1 (XXMFACC, "xxmfacc", QUAD, mma_xxmfacc)
> BU_MMA_1 (XXMTACC, "xxmtacc", QUAD, mma_xxmtacc)
> diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c
> index 2cf78dfa5fe..fc1671e1bb2 100644
> --- a/gcc/config/rs6000/rs6000-call.c
> +++ b/gcc/config/rs6000/rs6000-call.c
> @@ -13383,8 +13383,8 @@ builtin_function_type (machine_mode mode_ret, machine_mode mode_arg0,
> case P8V_BUILTIN_VGBBD:
> case MISC_BUILTIN_CDTBCD:
> case MISC_BUILTIN_CBCDTD:
> - case VSX_BUILTIN_XVCVSPBF16:
> - case VSX_BUILTIN_XVCVBF16SPN:
> + case P10V_BUILTIN_XVCVSPBF16:
> + case P10V_BUILTIN_XVCVBF16SPN:
> h.uns_p = 1;
> h.uns_p = 1;
More information about the Gcc-patches