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

will schmidt will_schmidt@vnet.ibm.com
Tue Aug 25 16:56:58 GMT 2020


On Mon, 2020-08-24 at 14:39 -0700, Carl Love wrote:
> Segher:
> 
> 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.


Hi, 

> 
> 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.
> 
> gcc/ChangeLog
> 
>             2020-08-24  Carl Love  <cel@us.ibm.com>

whitespace/indentation

> 	* 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 */
>  #endif
> 
>  
> @@ -2698,8 +2708,8 @@ BU_SPECIAL_X (RS6000_BUILTIN_CFSTRING, "__builtin_cfstring", RS6000_BTM_ALWAYS,
>  	      RS6000_BTC_MISC)
> 
>  /* 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[0] = 1;
>        h.uns_p[1] = 1;
>        break;
ok.


thanks,
-WIll




More information about the Gcc-patches mailing list