rs6000: Add undocumented switch -mprefixed-addr

Message ID 78d1bd8d-bdaf-bb90-4e71-b8e3b9746cdf@linux.ibm.com
State New
Headers show
Series
  • rs6000: Add undocumented switch -mprefixed-addr
Related show

Commit Message

Bill Schmidt May 29, 2019, 12:42 p.m.
Hi,

This patch adds the undocumented switch -mprefixed-addr and a little of the basic
infrastructure around it.  It also includes a couple of lines in the same code
regions to disable P8 fusion for -mcpu=future.

Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.
Is this okay for trunk?

Thanks,
Bill


2019-05-28  Bill Schmidt  <wschmidt@linux.ibm.com>
	    Michael Meissner  <meissner@linux.ibm.com>

	* rs6000-cpus.def (OTHER_FUSION_MASKS): New #define.
	(ISA_FUTURE_MASKS_SERVER): Add OPTION_MASK_PREFIXED_ADDR. Mask off
	OTHER_FUSION_MASKS.
	(OTHER_FUTURE_MASKS): Add OPTION_MASK_PREFIXED_ADDR.
	(POWERPC_MASKS): Likewise.
	* rs6000.c (rs6000_option_override_internal): Error if
	-mprefixed-addr is specified without -mcpu=future. Error if
	-mpcrel is specified without -mprefixed-addr.
	(rs6000_opt_masks): Add entry for prefixed-addr.
	* rs6000.opt (mprefixed-addr): New option.

Comments

Segher Boessenkool May 29, 2019, 1:16 p.m. | #1
Hi!

On Wed, May 29, 2019 at 07:42:38AM -0500, Bill Schmidt wrote:
> 	* rs6000-cpus.def (OTHER_FUSION_MASKS): New #define.

> 	(ISA_FUTURE_MASKS_SERVER): Add OPTION_MASK_PREFIXED_ADDR. Mask off

> 	OTHER_FUSION_MASKS.


Two spaces after a full stop (here and later again).

> +/* ISA masks setting fusion options.  */

> +#define OTHER_FUSION_MASKS	(OPTION_MASK_P8_FUSION			\

> +				 | OPTION_MASK_P8_FUSION_SIGN)


Or merge the two masks into one?

>  /* Support for a future processor's features.  */

> -#define ISA_FUTURE_MASKS_SERVER	(ISA_3_0_MASKS_SERVER			\

> -				 | OPTION_MASK_FUTURE			\

> -				 | OPTION_MASK_PCREL)

> +#define ISA_FUTURE_MASKS_SERVER	((ISA_3_0_MASKS_SERVER			\

> +				  | OPTION_MASK_FUTURE			\

> +				  | OPTION_MASK_PCREL			\

> +				  | OPTION_MASK_PREFIXED_ADDR)		\

> +				 & ~OTHER_FUSION_MASKS)


OTHER_FUSION_MASKS shouldn't be part of ISA_3_0_MASKS_SERVER.  Fix that
instead?  Fusion is a property of specific CPUs, not of ISA versions.

> -  /* -mpcrel requires the prefixed load/store support on FUTURE systems.  */

> -  if (!TARGET_FUTURE && TARGET_PCREL)

> +  /* -mprefixed-addr and -mpcrel require the prefixed load/store support on

> +     FUTURE systems.  */

> +  if (!TARGET_FUTURE && (TARGET_PCREL || TARGET_PREFIXED_ADDR))

>      {

>        if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)

>  	error ("%qs requires %qs", "-mpcrel", "-mcpu=future");


PCREL requires PREFIXED_ADDR, please simplify.

> +  if (TARGET_PCREL && !TARGET_PREFIXED_ADDR)

> +    {

> +      if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)

> +	error ("%qs requires %qs", "-mpcrel", "-mprefixed-addr");

> +

>        rs6000_isa_flags &= ~OPTION_MASK_PCREL;

>      }


Maybe put this test first, if that makes things easier or more logical?

> @@ -36379,6 +36391,7 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =

>    { "power9-vector",		OPTION_MASK_P9_VECTOR,		false, true  },

>    { "powerpc-gfxopt",		OPTION_MASK_PPC_GFXOPT,		false, true  },

>    { "powerpc-gpopt",		OPTION_MASK_PPC_GPOPT,		false, true  },

> +  { "prefixed-addr",		OPTION_MASK_PREFIXED_ADDR,	false, true  },


Do we want this?  Why?


Segher
Bill Schmidt May 29, 2019, 8:26 p.m. | #2
On 5/29/19 8:16 AM, Segher Boessenkool wrote:
> Hi!

>

> On Wed, May 29, 2019 at 07:42:38AM -0500, Bill Schmidt wrote:

>> 	* rs6000-cpus.def (OTHER_FUSION_MASKS): New #define.

>> 	(ISA_FUTURE_MASKS_SERVER): Add OPTION_MASK_PREFIXED_ADDR. Mask off

>> 	OTHER_FUSION_MASKS.

> Two spaces after a full stop (here and later again).


Oops, yep.
>

>> +/* ISA masks setting fusion options.  */

>> +#define OTHER_FUSION_MASKS	(OPTION_MASK_P8_FUSION			\

>> +				 | OPTION_MASK_P8_FUSION_SIGN)

> Or merge the two masks into one?


I'll ask Mike to explain this, as I don't know why there are two masks.
>

>>  /* Support for a future processor's features.  */

>> -#define ISA_FUTURE_MASKS_SERVER	(ISA_3_0_MASKS_SERVER			\

>> -				 | OPTION_MASK_FUTURE			\

>> -				 | OPTION_MASK_PCREL)

>> +#define ISA_FUTURE_MASKS_SERVER	((ISA_3_0_MASKS_SERVER			\

>> +				  | OPTION_MASK_FUTURE			\

>> +				  | OPTION_MASK_PCREL			\

>> +				  | OPTION_MASK_PREFIXED_ADDR)		\

>> +				 & ~OTHER_FUSION_MASKS)

> OTHER_FUSION_MASKS shouldn't be part of ISA_3_0_MASKS_SERVER.  Fix that

> instead?  Fusion is a property of specific CPUs, not of ISA versions.


Agreed, I think this should be masked out of ISA_3_0_MASKS_SERVER, but
would like Mike to agree before I change it in case I'm missing
something obvious.
>

>> -  /* -mpcrel requires the prefixed load/store support on FUTURE systems.  */

>> -  if (!TARGET_FUTURE && TARGET_PCREL)

>> +  /* -mprefixed-addr and -mpcrel require the prefixed load/store support on

>> +     FUTURE systems.  */

>> +  if (!TARGET_FUTURE && (TARGET_PCREL || TARGET_PREFIXED_ADDR))

>>      {

>>        if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)

>>  	error ("%qs requires %qs", "-mpcrel", "-mcpu=future");

> PCREL requires PREFIXED_ADDR, please simplify.

>

>> +  if (TARGET_PCREL && !TARGET_PREFIXED_ADDR)

>> +    {

>> +      if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)

>> +	error ("%qs requires %qs", "-mpcrel", "-mprefixed-addr");

>> +

>>        rs6000_isa_flags &= ~OPTION_MASK_PCREL;

>>      }

> Maybe put this test first, if that makes things easier or more logical?

ok
>

>> @@ -36379,6 +36391,7 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =

>>    { "power9-vector",		OPTION_MASK_P9_VECTOR,		false, true  },

>>    { "powerpc-gfxopt",		OPTION_MASK_PPC_GFXOPT,		false, true  },

>>    { "powerpc-gpopt",		OPTION_MASK_PPC_GPOPT,		false, true  },

>> +  { "prefixed-addr",		OPTION_MASK_PREFIXED_ADDR,	false, true  },

> Do we want this?  Why?


Performance folks are using it for testing purposes.  Eventually this
will probably drop out, but for now I think it's best to have the
undocumented switch.

Thanks,
Bill
>

>

> Segher

>
Segher Boessenkool May 29, 2019, 9:33 p.m. | #3
On Wed, May 29, 2019 at 03:26:27PM -0500, Bill Schmidt wrote:
> >> @@ -36379,6 +36391,7 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =

> >>    { "power9-vector",		OPTION_MASK_P9_VECTOR,		false, true  },

> >>    { "powerpc-gfxopt",		OPTION_MASK_PPC_GFXOPT,		false, true  },

> >>    { "powerpc-gpopt",		OPTION_MASK_PPC_GPOPT,		false, true  },

> >> +  { "prefixed-addr",		OPTION_MASK_PREFIXED_ADDR,	false, true  },

> > Do we want this?  Why?

> 

> Performance folks are using it for testing purposes.  Eventually this

> will probably drop out, but for now I think it's best to have the

> undocumented switch.


Command-line options, sure, but is that what this code is about?  Huh.
I thought it was for attribute target and that stuff.

Oh well, okay either way.  For now :-)


Segher
Michael Meissner May 30, 2019, 4:01 p.m. | #4
On Wed, May 29, 2019 at 03:26:27PM -0500, Bill Schmidt wrote:
> On 5/29/19 8:16 AM, Segher Boessenkool wrote:

> > Hi!

> >

> > On Wed, May 29, 2019 at 07:42:38AM -0500, Bill Schmidt wrote:

> >> 	* rs6000-cpus.def (OTHER_FUSION_MASKS): New #define.

> >> 	(ISA_FUTURE_MASKS_SERVER): Add OPTION_MASK_PREFIXED_ADDR. Mask off

> >> 	OTHER_FUSION_MASKS.

> > Two spaces after a full stop (here and later again).

> 

> Oops, yep.

> >

> >> +/* ISA masks setting fusion options.  */

> >> +#define OTHER_FUSION_MASKS	(OPTION_MASK_P8_FUSION			\

> >> +				 | OPTION_MASK_P8_FUSION_SIGN)

> > Or merge the two masks into one?

> 

> I'll ask Mike to explain this, as I don't know why there are two masks.


The intention is to allow for using the numeric prefixed instructions without
pc-relative.  I.e.

	long c = 0x12345;

would generate:

	PLI r,0x12345

Instead of ADDI/ADDIS, but still not generate the pc-relative instructions.
The intention was when we get to timing stuff, there are fewer differences.  I
could live with dropping prefixed-addr as a separate switch.

However, since there are other prefixed instructions that we will do someday
that aren't necessarily using pc-relative, if we drop it, we need to make sure
all of the prefixed stuff is now targetted on -mfuture instead of
-mprefixed-addr.

> >

> >>  /* Support for a future processor's features.  */

> >> -#define ISA_FUTURE_MASKS_SERVER	(ISA_3_0_MASKS_SERVER			\

> >> -				 | OPTION_MASK_FUTURE			\

> >> -				 | OPTION_MASK_PCREL)

> >> +#define ISA_FUTURE_MASKS_SERVER	((ISA_3_0_MASKS_SERVER			\

> >> +				  | OPTION_MASK_FUTURE			\

> >> +				  | OPTION_MASK_PCREL			\

> >> +				  | OPTION_MASK_PREFIXED_ADDR)		\

> >> +				 & ~OTHER_FUSION_MASKS)

> > OTHER_FUSION_MASKS shouldn't be part of ISA_3_0_MASKS_SERVER.  Fix that

> > instead?  Fusion is a property of specific CPUs, not of ISA versions.

> 

> Agreed, I think this should be masked out of ISA_3_0_MASKS_SERVER, but

> would like Mike to agree before I change it in case I'm missing

> something obvious.


Mostly it was me being cautious that I wasn't sure all of the p8 fusion stuff
would play well with prefixed addressing (since the p8 fusion stuff does use
peephole2 and it might not be prepared for prefixed instructions).

> >

> >> -  /* -mpcrel requires the prefixed load/store support on FUTURE systems.  */

> >> -  if (!TARGET_FUTURE && TARGET_PCREL)

> >> +  /* -mprefixed-addr and -mpcrel require the prefixed load/store support on

> >> +     FUTURE systems.  */

> >> +  if (!TARGET_FUTURE && (TARGET_PCREL || TARGET_PREFIXED_ADDR))

> >>      {

> >>        if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)

> >>  	error ("%qs requires %qs", "-mpcrel", "-mcpu=future");

> > PCREL requires PREFIXED_ADDR, please simplify.

> >

> >> +  if (TARGET_PCREL && !TARGET_PREFIXED_ADDR)

> >> +    {

> >> +      if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)

> >> +	error ("%qs requires %qs", "-mpcrel", "-mprefixed-addr");

> >> +

> >>        rs6000_isa_flags &= ~OPTION_MASK_PCREL;

> >>      }

> > Maybe put this test first, if that makes things easier or more logical?

> ok


Yes, I was working on this with my no-pcrel default patch I was about to submit.

> >

> >> @@ -36379,6 +36391,7 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =

> >>    { "power9-vector",		OPTION_MASK_P9_VECTOR,		false, true  },

> >>    { "powerpc-gfxopt",		OPTION_MASK_PPC_GFXOPT,		false, true  },

> >>    { "powerpc-gpopt",		OPTION_MASK_PPC_GPOPT,		false, true  },

> >> +  { "prefixed-addr",		OPTION_MASK_PREFIXED_ADDR,	false, true  },

> > Do we want this?  Why?

> 

> Performance folks are using it for testing purposes.  Eventually this

> will probably drop out, but for now I think it's best to have the

> undocumented switch.


I use that table with -mdebug=reg so I can make sure exactly what options are
on or off.  Please add any undocumented switch to the table.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797
Segher Boessenkool June 2, 2019, 11:01 p.m. | #5
On Thu, May 30, 2019 at 12:01:45PM -0400, Michael Meissner wrote:
> On Wed, May 29, 2019 at 03:26:27PM -0500, Bill Schmidt wrote:

> > On 5/29/19 8:16 AM, Segher Boessenkool wrote:

> > >> +/* ISA masks setting fusion options.  */

> > >> +#define OTHER_FUSION_MASKS	(OPTION_MASK_P8_FUSION			\

> > >> +				 | OPTION_MASK_P8_FUSION_SIGN)

> > > Or merge the two masks into one?

> > 

> > I'll ask Mike to explain this, as I don't know why there are two masks.

> 

> The intention is to allow for using the numeric prefixed instructions without

> pc-relative.  I.e.


[ snip ]

I was suggesting merging these two P8_FUSION{,_SIGN} into one.  But, we'll
get to that some day, it doesn't have to be now.

> > >> @@ -36379,6 +36391,7 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =

> > >>    { "power9-vector",		OPTION_MASK_P9_VECTOR,		false, true  },

> > >>    { "powerpc-gfxopt",		OPTION_MASK_PPC_GFXOPT,		false, true  },

> > >>    { "powerpc-gpopt",		OPTION_MASK_PPC_GPOPT,		false, true  },

> > >> +  { "prefixed-addr",		OPTION_MASK_PREFIXED_ADDR,	false, true  },

> > > Do we want this?  Why?

> > 

> > Performance folks are using it for testing purposes.  Eventually this

> > will probably drop out, but for now I think it's best to have the

> > undocumented switch.

> 

> I use that table with -mdebug=reg so I can make sure exactly what options are

> on or off.  Please add any undocumented switch to the table.


It's not very nice to have to edit everything in two completely separate
places like this.


Segher

Patch

diff --git a/gcc/config/rs6000/rs6000-cpus.def b/gcc/config/rs6000/rs6000-cpus.def
index 5337382bdcf..2fb612a8401 100644
--- a/gcc/config/rs6000/rs6000-cpus.def
+++ b/gcc/config/rs6000/rs6000-cpus.def
@@ -72,13 +72,20 @@ 
 				 | OPTION_MASK_P9_VECTOR		\
 				 | OPTION_MASK_DIRECT_MOVE)
 
+/* ISA masks setting fusion options.  */
+#define OTHER_FUSION_MASKS	(OPTION_MASK_P8_FUSION			\
+				 | OPTION_MASK_P8_FUSION_SIGN)
+
 /* Support for a future processor's features.  */
-#define ISA_FUTURE_MASKS_SERVER	(ISA_3_0_MASKS_SERVER			\
-				 | OPTION_MASK_FUTURE			\
-				 | OPTION_MASK_PCREL)
+#define ISA_FUTURE_MASKS_SERVER	((ISA_3_0_MASKS_SERVER			\
+				  | OPTION_MASK_FUTURE			\
+				  | OPTION_MASK_PCREL			\
+				  | OPTION_MASK_PREFIXED_ADDR)		\
+				 & ~OTHER_FUSION_MASKS)
 
 /* Flags that need to be turned off if -mno-future.  */
-#define OTHER_FUTURE_MASKS	(OPTION_MASK_PCREL)
+#define OTHER_FUTURE_MASKS	(OPTION_MASK_PCREL			\
+				 | OPTION_MASK_PREFIXED_ADDR)
 
 /* Flags that need to be turned off if -mno-power9-vector.  */
 #define OTHER_P9_VECTOR_MASKS	(OPTION_MASK_FLOAT128_HW		\
@@ -139,6 +146,7 @@ 
 				 | OPTION_MASK_POWERPC64		\
 				 | OPTION_MASK_PPC_GFXOPT		\
 				 | OPTION_MASK_PPC_GPOPT		\
+				 | OPTION_MASK_PREFIXED_ADDR		\
 				 | OPTION_MASK_QUAD_MEMORY		\
 				 | OPTION_MASK_QUAD_MEMORY_ATOMIC	\
 				 | OPTION_MASK_RECIP_PRECISION		\
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 9229bad6acc..1860b344c3a 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -4296,12 +4296,24 @@  rs6000_option_override_internal (bool global_init_p)
       rs6000_isa_flags &= ~OPTION_MASK_FLOAT128_HW;
     }
 
-  /* -mpcrel requires the prefixed load/store support on FUTURE systems.  */
-  if (!TARGET_FUTURE && TARGET_PCREL)
+  /* -mprefixed-addr and -mpcrel require the prefixed load/store support on
+     FUTURE systems.  */
+  if (!TARGET_FUTURE && (TARGET_PCREL || TARGET_PREFIXED_ADDR))
     {
       if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)
 	error ("%qs requires %qs", "-mpcrel", "-mcpu=future");
 
+      else if ((rs6000_isa_flags_explicit & OPTION_MASK_PREFIXED_ADDR) != 0)
+	error ("%qs requires %qs", "-mprefixed-addr", "-mcpu=future");
+
+      rs6000_isa_flags &= ~(OPTION_MASK_PCREL | OPTION_MASK_PREFIXED_ADDR);
+    }
+
+  if (TARGET_PCREL && !TARGET_PREFIXED_ADDR)
+    {
+      if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)
+	error ("%qs requires %qs", "-mpcrel", "-mprefixed-addr");
+
       rs6000_isa_flags &= ~OPTION_MASK_PCREL;
     }
 
@@ -36379,6 +36391,7 @@  static struct rs6000_opt_mask const rs6000_opt_masks[] =
   { "power9-vector",		OPTION_MASK_P9_VECTOR,		false, true  },
   { "powerpc-gfxopt",		OPTION_MASK_PPC_GFXOPT,		false, true  },
   { "powerpc-gpopt",		OPTION_MASK_PPC_GPOPT,		false, true  },
+  { "prefixed-addr",		OPTION_MASK_PREFIXED_ADDR,	false, true  },
   { "quad-memory",		OPTION_MASK_QUAD_MEMORY,	false, true  },
   { "quad-memory-atomic",	OPTION_MASK_QUAD_MEMORY_ATOMIC,	false, true  },
   { "recip-precision",		OPTION_MASK_RECIP_PRECISION,	false, true  },
diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt
index 43b04834746..3a4353674b8 100644
--- a/gcc/config/rs6000/rs6000.opt
+++ b/gcc/config/rs6000/rs6000.opt
@@ -574,6 +574,10 @@  mfuture
 Target Report Mask(FUTURE) Var(rs6000_isa_flags)
 Use instructions for a future architecture.
 
+mprefixed-addr
+Target Undocumented Mask(PREFIXED_ADDR) Var(rs6000_isa_flags)
+Generate (do not generate) prefixed memory instructions.
+
 mpcrel
 Target Report Mask(PCREL) Var(rs6000_isa_flags)
 Generate (do not generate) pc-relative memory addressing.