diff mbox series

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

Commit Message

Bill Schmidt May 29, 2019, 12:42 p.m. UTC 
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. UTC | #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. UTC | #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. UTC | #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. UTC | #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.
Segher Boessenkool June 2, 2019, 11:01 p.m. UTC | #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
diff mbox series

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.