[PATCH] rs6000, restrict bfloat convert intrinsic to Power 10. Fix BU_P10V macro definitions.

Segher Boessenkool segher@kernel.crashing.org
Sat Aug 15 00:42:33 GMT 2020


Hi!

On Fri, Aug 14, 2020 at 03:32:47PM -0700, Carl Love wrote:
> On Fri, 2020-08-14 at 16:33 -0500, Segher Boessenkool wrote:
> > 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.

Excellent :-)

> > Do the names agree with the (future) documentation now?
> 
> Did not double check on the documentation.

Someone should...

> > > -#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.

Ah, okay, yes let's keep it all consistent then, to make the rewrite
easier :-)

> 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.....

It's __builtin_ vs. __builtin_vec_.

> Thoughts?  I will change to whatever everyone agrees on.

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!


Segher


More information about the Gcc-patches mailing list