[PATCH, rs6000] correct an erroneous BTM value in the BU_P10_MISC define

[PATCH, rs6000] correct an erroneous BTM value in the BU_P10_MISC define

[will schmidt]

[PATCH, rs6000] correct an erroneous blip in the BU_P10_MISC define

Hi, 
    We have extraneous BTM entry (RS6000_BTM_POWERPC64) in the define for
our P10 MISC 2 builtin definition.  This does not exist for the '0',
'1' or '3' definitions. It appears to me that this was erroneously
copied from the P7 version of the define which contains a version of the
BU macro both with and without that element.  Removing the
RS6000_BTM_POWERPC64 portion of the define does not introduce any obvious
failures, I believe this extra line can be safely removed.

OK for trunk?

Thanks
-Will

diff --git a/gcc/config/rs6000/rs6000-builtin.def b/gcc/config/rs6000/rs6000-builtin.def
index e91a48ddf5fe..62c9b77cb76d 100644
--- a/gcc/config/rs6000/rs6000-builtin.def
+++ b/gcc/config/rs6000/rs6000-builtin.def
@@ -1112,12 +1112,11 @@
 		    CODE_FOR_ ## ICODE)			/* ICODE */
 
 #define BU_P10_MISC_2(ENUM, NAME, ATTR, ICODE)				\
   RS6000_BUILTIN_2 (P10_BUILTIN_ ## ENUM,		/* ENUM */	\
 		    "__builtin_" NAME,			/* NAME */	\
-		    RS6000_BTM_P10					\
-		    | RS6000_BTM_POWERPC64,		/* MASK */	\
+		    RS6000_BTM_P10,			/* MASK */	\
 		    (RS6000_BTC_ ## ATTR		/* ATTR */	\
 		     | RS6000_BTC_BINARY),				\
 		    CODE_FOR_ ## ICODE)			/* ICODE */
 
 #define BU_P10_MISC_3(ENUM, NAME, ATTR, ICODE)				\

Re: [PATCH, rs6000] correct an erroneous BTM value in the BU_P10_MISC define

[Segher Boessenkool]

Hi!

On Thu, Sep 24, 2020 at 03:35:24PM -0500, will schmidt wrote:
>     We have extraneous BTM entry (RS6000_BTM_POWERPC64) in the define for
> our P10 MISC 2 builtin definition.  This does not exist for the '0',
> '1' or '3' definitions. It appears to me that this was erroneously
> copied from the P7 version of the define which contains a version of the
> BU macro both with and without that element.  Removing the
> RS6000_BTM_POWERPC64 portion of the define does not introduce any obvious
> failures, I believe this extra line can be safely removed.

No, it cannot.

This is used for pdepd/pextd/cntlzdm/cnttzdm/cfuged, all of which do
need 64-bit registers to do anything sane.

This should really have defined some new builtin class, and I thought we
could just be tricky and take a massive shortcut.  Bill has been hit by
this already as well, sigh :-(


Segher

Re: [PATCH, rs6000] correct an erroneous BTM value in the BU_P10_MISC define

[will schmidt]

On Fri, 2020-09-25 at 12:36 -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, Sep 24, 2020 at 03:35:24PM -0500, will schmidt wrote:
> >     We have extraneous BTM entry (RS6000_BTM_POWERPC64) in the
> > define for
> > our P10 MISC 2 builtin definition.  This does not exist for the
> > '0',
> > '1' or '3' definitions. It appears to me that this was erroneously
> > copied from the P7 version of the define which contains a version
> > of the
> > BU macro both with and without that element.  Removing the
> > RS6000_BTM_POWERPC64 portion of the define does not introduce any
> > obvious
> > failures, I believe this extra line can be safely removed.
> 
> No, it cannot.
> 
> This is used for pdepd/pextd/cntlzdm/cnttzdm/cfuged, all of which do
> need 64-bit registers to do anything sane.
> 
> This should really have defined some new builtin class, and I thought
> we
> could just be tricky and take a massive shortcut.  Bill has been hit
> by
> this already as well, sigh :-(

Ok.

The usage of that macro seems to be limited to those that you have
referenced.  i.e. 

/* Builtins for scalar instructions added in ISA 3.1 (power10).  */
BU_P10_MISC_2 (CFUGED, "cfuged", CONST, cfuged)
BU_P10_MISC_2 (CNTLZDM, "cntlzdm", CONST, cntlzdm)
BU_P10_MISC_2 (CNTTZDM, "cnttzdm", CONST, cnttzdm)
BU_P10_MISC_2 (PDEPD, "pdepd", CONST, pdepd)
BU_P10_MISC_2 (PEXTD, "pextd", CONST, pextd)

So looking at the power7 entries that have the BTM_POWERPC64 entry..

BU_P7_MISC_2 (DIVWE,		"divwe",	CONST,	dive_si)
BU_P7_MISC_2 (DIVWEU,		"divweu",	CONST,	diveu_si)
BU_P7_POWERPC64_MISC_2 (DIVDE,	"divde",	CONST,	dive_di)
BU_P7_POWERPC64_MISC_2 (DIVDEU,	"divdeu",	CONST,	diveu_di)

Would it be suitable to rename the P10 macro to 
BU_P10_POWERPC64_MISC_2 ? 

I'd then debate whether to add a unused macro to fill the gap between
BU_P10_MISC_1 and BU_P10_MISC_2

If you've got schemes for a deeper fix, i'd need another hint. :-)

thanks
-Will

> 
> 
> Segher

Re: [PATCH, rs6000] correct an erroneous BTM value in the BU_P10_MISC define

[Segher Boessenkool]

On Fri, Sep 25, 2020 at 03:34:49PM -0500, will schmidt wrote:
> On Fri, 2020-09-25 at 12:36 -0500, Segher Boessenkool wrote:
> > No, it cannot.
> > 
> > This is used for pdepd/pextd/cntlzdm/cnttzdm/cfuged, all of which do
> > need 64-bit registers to do anything sane.
> > 
> > This should really have defined some new builtin class, and I thought
> > we
> > could just be tricky and take a massive shortcut.  Bill has been hit
> > by
> > this already as well, sigh :-(
> 
> Ok.
> 
> The usage of that macro seems to be limited to those that you have
> referenced.  i.e. 
> 
> /* Builtins for scalar instructions added in ISA 3.1 (power10).  */
> BU_P10_MISC_2 (CFUGED, "cfuged", CONST, cfuged)
> BU_P10_MISC_2 (CNTLZDM, "cntlzdm", CONST, cntlzdm)
> BU_P10_MISC_2 (CNTTZDM, "cnttzdm", CONST, cnttzdm)
> BU_P10_MISC_2 (PDEPD, "pdepd", CONST, pdepd)
> BU_P10_MISC_2 (PEXTD, "pextd", CONST, pextd)
> 
> So looking at the power7 entries that have the BTM_POWERPC64 entry..
> 
> BU_P7_MISC_2 (DIVWE,		"divwe",	CONST,	dive_si)
> BU_P7_MISC_2 (DIVWEU,		"divweu",	CONST,	diveu_si)
> BU_P7_POWERPC64_MISC_2 (DIVDE,	"divde",	CONST,	dive_di)
> BU_P7_POWERPC64_MISC_2 (DIVDEU,	"divdeu",	CONST,	diveu_di)
> 
> Would it be suitable to rename the P10 macro to 
> BU_P10_POWERPC64_MISC_2 ? 

Yes.  But that requires some more infrastructure I thought...  Maybe not
though?  And we can do that anyway of course, it's not like we do not
have way way way too much there already.

> I'd then debate whether to add a unused macro to fill the gap between
> BU_P10_MISC_1 and BU_P10_MISC_2

Nah, don't bother, those are just names, the numbers are meaningless :-)

> If you've got schemes for a deeper fix, i'd need another hint. :-)

Talk with Bill if this makes things easier for him / harder / no
difference?

Thanks,


Segher

Re: [PATCH, rs6000] correct an erroneous BTM value in the BU_P10_MISC define

[Bill Schmidt]

On 9/25/20 6:50 PM, Segher Boessenkool wrote:
> On Fri, Sep 25, 2020 at 03:34:49PM -0500, will schmidt wrote:
>> On Fri, 2020-09-25 at 12:36 -0500, Segher Boessenkool wrote:
>>> No, it cannot.
>>>
>>> This is used for pdepd/pextd/cntlzdm/cnttzdm/cfuged, all of which do
>>> need 64-bit registers to do anything sane.
>>>
>>> This should really have defined some new builtin class, and I thought
>>> we
>>> could just be tricky and take a massive shortcut.  Bill has been hit
>>> by
>>> this already as well, sigh :-(
>> Ok.
>>
>> The usage of that macro seems to be limited to those that you have
>> referenced.  i.e.
>>
>> /* Builtins for scalar instructions added in ISA 3.1 (power10).  */
>> BU_P10_MISC_2 (CFUGED, "cfuged", CONST, cfuged)
>> BU_P10_MISC_2 (CNTLZDM, "cntlzdm", CONST, cntlzdm)
>> BU_P10_MISC_2 (CNTTZDM, "cnttzdm", CONST, cnttzdm)
>> BU_P10_MISC_2 (PDEPD, "pdepd", CONST, pdepd)
>> BU_P10_MISC_2 (PEXTD, "pextd", CONST, pextd)
>>
>> So looking at the power7 entries that have the BTM_POWERPC64 entry..
>>
>> BU_P7_MISC_2 (DIVWE,		"divwe",	CONST,	dive_si)
>> BU_P7_MISC_2 (DIVWEU,		"divweu",	CONST,	diveu_si)
>> BU_P7_POWERPC64_MISC_2 (DIVDE,	"divde",	CONST,	dive_di)
>> BU_P7_POWERPC64_MISC_2 (DIVDEU,	"divdeu",	CONST,	diveu_di)
>>
>> Would it be suitable to rename the P10 macro to
>> BU_P10_POWERPC64_MISC_2 ?
> Yes.  But that requires some more infrastructure I thought...  Maybe not
> though?  And we can do that anyway of course, it's not like we do not
> have way way way too much there already.
>
>> I'd then debate whether to add a unused macro to fill the gap between
>> BU_P10_MISC_1 and BU_P10_MISC_2
> Nah, don't bother, those are just names, the numbers are meaningless :-)
>
>> If you've got schemes for a deeper fix, i'd need another hint. :-)
> Talk with Bill if this makes things easier for him / harder / no
> difference?

What Will has in mind is what I would prefer.  I identified this as a 
naming problem above all else.  The only issue for me is that I nearly 
missed it when converting things to use the new builtin methodology, 
because it wasn't made obvious by the naming.

Thanks,

Bill

>
> Thanks,
>
>
> Segher

[PATCH, rs6000] rename BU_P10_MISC_2 define to BU_P10_POWERPC64_MISC_2

[will schmidt]

Hi,
  Rename our BU_P10_MISC_2 built-in define macro to be
BU_P10_POWERPC64_MISC_2.   This more accurately reflects
that the macro includes the RS6000_BTM_POWERPC64 entry
that is not present in the other BU_P10_MISC macros, 
and matches the style we used for the P7 equivalent.

Should be entirely cosmetic, no codegen changes.
A regtest is underway just in case.
OK for trunk?

Thanks,
-Will
    
    gcc/ChangeLog:
            * gcc/config/rs6000/rs6000-builtin.def (BU_P10_MISC_2): Rename
            to BU_P10_POWERPC64_MISC_2.
            (CFUGED,CNTLZDM,CNTTZDM,PDEPD,PEXTD): Call renamed macro.

diff --git a/gcc/config/rs6000/rs6000-builtin.def b/gcc/config/rs6000/rs6000-builtin.def
index e91a48ddf5fe..3eb55f0ae434 100644
--- a/gcc/config/rs6000/rs6000-builtin.def
+++ b/gcc/config/rs6000/rs6000-builtin.def
@@ -1109,11 +1109,11 @@
 		    RS6000_BTM_P10,			/* MASK */	\
 		    (RS6000_BTC_ ## ATTR		/* ATTR */	\
 		     | RS6000_BTC_UNARY),				\
 		    CODE_FOR_ ## ICODE)			/* ICODE */
 
-#define BU_P10_MISC_2(ENUM, NAME, ATTR, ICODE)				\
+#define BU_P10_POWERPC64_MISC_2(ENUM, NAME, ATTR, ICODE)		\
   RS6000_BUILTIN_2 (P10_BUILTIN_ ## ENUM,		/* ENUM */	\
 		    "__builtin_" NAME,			/* NAME */	\
 		    RS6000_BTM_P10					\
 		    | RS6000_BTM_POWERPC64,		/* MASK */	\
 		    (RS6000_BTC_ ## ATTR		/* ATTR */	\
@@ -2725,15 +2725,15 @@ BU_P9_64BIT_2 (CMPEQB,	"byte_in_set",	CONST,	cmpeqb)
 BU_P9_OVERLOAD_2 (CMPRB,	"byte_in_range")
 BU_P9_OVERLOAD_2 (CMPRB2,	"byte_in_either_range")
 BU_P9_OVERLOAD_2 (CMPEQB,	"byte_in_set")
 \f
 /* Builtins for scalar instructions added in ISA 3.1 (power10).  */
-BU_P10_MISC_2 (CFUGED, "cfuged", CONST, cfuged)
-BU_P10_MISC_2 (CNTLZDM, "cntlzdm", CONST, cntlzdm)
-BU_P10_MISC_2 (CNTTZDM, "cnttzdm", CONST, cnttzdm)
-BU_P10_MISC_2 (PDEPD, "pdepd", CONST, pdepd)
-BU_P10_MISC_2 (PEXTD, "pextd", CONST, pextd)
+BU_P10_POWERPC64_MISC_2 (CFUGED, "cfuged", CONST, cfuged)
+BU_P10_POWERPC64_MISC_2 (CNTLZDM, "cntlzdm", CONST, cntlzdm)
+BU_P10_POWERPC64_MISC_2 (CNTTZDM, "cnttzdm", CONST, cnttzdm)
+BU_P10_POWERPC64_MISC_2 (PDEPD, "pdepd", CONST, pdepd)
+BU_P10_POWERPC64_MISC_2 (PEXTD, "pextd", CONST, pextd)
 
 /* Builtins for vector instructions added in ISA 3.1 (power10).  */
 BU_P10V_AV_2 (VCLRLB, "vclrlb", CONST, vclrlb)
 BU_P10V_AV_2 (VCLRRB, "vclrrb", CONST, vclrrb)
 BU_P10V_AV_2 (VCFUGED, "vcfuged", CONST, vcfuged)

Re: [PATCH, rs6000] rename BU_P10_MISC_2 define to BU_P10_POWERPC64_MISC_2

[Segher Boessenkool]

Hi!

On Wed, Oct 07, 2020 at 12:44:04PM -0500, will schmidt wrote:
>   Rename our BU_P10_MISC_2 built-in define macro to be
> BU_P10_POWERPC64_MISC_2.   This more accurately reflects
> that the macro includes the RS6000_BTM_POWERPC64 entry
> that is not present in the other BU_P10_MISC macros, 
> and matches the style we used for the P7 equivalent.
> 
> Should be entirely cosmetic, no codegen changes.
> A regtest is underway just in case.

This is okay for trunk.  Thank you!


>             * gcc/config/rs6000/rs6000-builtin.def (BU_P10_MISC_2): Rename
>             to BU_P10_POWERPC64_MISC_2.
>             (CFUGED,CNTLZDM,CNTTZDM,PDEPD,PEXTD): Call renamed macro.

(space after comma?)


Segher