Re: [PATCH] rs6000: Add undocumented switch -mprefixed-addr

[Bill Schmidt <wschmidt@linux.ibm.com>]

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
>