Message ID | 78d1bd8d-bdaf-bb90-4e71-b8e3b9746cdf@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | rs6000: Add undocumented switch -mprefixed-addr | expand |
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
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 >
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
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.
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 --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.