On Fri, 2020-08-14 at 16:33 -0500, Segher Boessenkool wrote:
> Hi Carl,
>
> On Thu, Aug 13, 2020 at 09:12:48AM -0700, Carl Love wrote:
> > The macro expansion for the bfloat convert intrinsics XVCVBF16SP
> > and
> > XVCVSPBF16 need to be restricted to P10.
> > The following patch creates new macro expansions BU_P10V_VSX_# and
> > BU_P10V_AV_# for the VSX and Altivec instructions
> > respectively. The
> > new names are consistent with the P8 and P9 naming convention for
> > the
> > VSX and Altivec instructions.
>
> So _vsx if it is for all VSRs, but _altivec for just the VRs?
Yes, I worked off the rule that Altivec registers are designated with
5-bits and VSR registers are designated with 6-bits.
>
> > The macro expansion for XVCVBF16SP and XVCVSPBF16 is changed from
> > BU_VSX_1 to BU_P10V_VSX_1 to restrict it to P10 and beyond. Also
> > MISC
> > is changed to CONST in the macro expansion call.
>
> The spelling of the xvcvbf16sp name will probably change, fwiw. So
> you
> might want to wait before committing this (but changing it later is
> fine
> as well).
OK, not sure of the urgency to get this patch in. Bill noted the error
recently in the expansion. Will check with him to see if we should
wait and go with the new name or go with the old name for now.
<snip>
>
> Do the names agree with the (future) documentation now?
Did not double check on the documentation.
>
> > -#define BU_P10V_1(ENUM, NAME, ATTR, ICODE)
> > \
> > - RS6000_BUILTIN_1 (P10_BUILTIN_ ## ENUM, /* ENUM */ \
> > - "__builtin_altivec_" NAME, /* NAME */
> > \
> > +#define BU_P10V_VSX_1(ENUM, NAME, ATTR, ICODE)
> > \
>
> Hrm, this now says "V" (for vector) as well as "VSX" (for all 64
> vector
> regs allowed). One of those is superfluous.
>
> > + RS6000_BUILTIN_1 (P10V_BUILTIN_ ## ENUM, /* ENUM */ \
>
> So this enum name doesn't say it allows all 64 registers?
>
> > + "__builtin_vsx_" NAME, /* NAME */ \
>
>
> > /* Builtins for vector instructions added in ISA 3.1
> > (power10). */
> > -BU_P10V_2 (VCLRLB, "vclrlb", CONST, vclrlb)
> > +BU_P10V_AV_2 (VCLRLB, "vclrlb", CONST, vclrlb)
>
> Maybe you should just keep "V" for insns using only the VRs (which
> you
> call V_AV now), and do "VS" for those working on all VSRs (which you
> call
> V_VSX here)?
The names BU_P10V_AV_# and BU_P10V_VSX_# were used for consistency with
the Power 8 and Power 9 naming, per my discussion with Peter. The
following already exist in rs6000-builtin.def for similar uses.
#define BU_P8V_AV_1(ENUM, NAME, ATTR, ICODE)
#define BU_P8V_AV_2(ENUM, NAME, ATTR, ICODE)
#define BU_P8V_AV_3(ENUM, NAME, ATTR, ICODE)
#define BU_P9V_AV_1(ENUM, NAME, ATTR, ICODE)
#define BU_P9V_AV_2(ENUM, NAME, ATTR, ICODE)
#define BU_P9V_AV_3(ENUM, NAME, ATTR, ICODE)
#define BU_P8V_VSX_1(ENUM, NAME, ATTR, ICODE)
#define BU_P8V_VSX_2(ENUM, NAME, ATTR, ICODE)
#define BU_P9V_VSX_1(ENUM, NAME, ATTR, ICODE)
#define BU_P9V_VSX_2(ENUM, NAME, ATTR, ICODE)
#define BU_P9V_VSX_3(ENUM, NAME, ATTR, ICODE)
I agree the V seems redundant given the VSX, AV in the name.
The overload macros:
#define BU_P8V_OVERLOAD_1(ENUM, NAME)
#define BU_P8V_OVERLOAD_2(ENUM, NAME)
#define BU_P8V_OVERLOAD_3(ENUM, NAME)
#define BU_P9_OVERLOAD_2(ENUM, NAME)
#define BU_P9V_OVERLOAD_1(ENUM, NAME)
#define BU_P9V_OVERLOAD_2(ENUM, NAME)
#define BU_P9V_OVERLOAD_3(ENUM, NAME)
Looks like BU_P9_OVERLOAD_2 and BU_P9V_OVERLOAD_2 are identical
definitions. That should probably be fixed.....
Seems like you need V in the processor name for the OVERLOAD
definitions to make it clear they are vector definitions. I don't
believe the OVERLOAD cares if the builtin is for vsx or altivec
registers. It kinda makes sense to leave the V in BU_P8V_VSX_#,
BU_P9V_VSX_#, BU_P8V_AV_#, BU_P9V_AV_# definitions for consistency with
the OVERLOAD macro name? Thoughts? I will change to whatever everyone
agrees on.
Carl
