Re: [PATCH] rs6000: Neuter option -mpower{8,9}-vector [PR109987]

["Kewen.Lin" <linkw@linux.ibm.com>]

on 2024/2/20 19:19, Segher Boessenkool wrote:
> On Tue, Feb 20, 2024 at 05:27:07PM +0800, Kewen.Lin wrote:
>> Good question, it mainly follows the practice of option direct-move here.
>> IMHO at least for power8-vector we want WarnRemoved for now as it's
>> documented before, and we can probably make it (or them) removed later on
>> trunk once all active branch releases don't support it any more.
>>
>> What's your opinion on this?
> 
> Originally I did
>   Warn(%qs is deprecated)
> which already was a mistake.  It then changed to
>   Deprecated
> and then to
>   WarnRemoved
> which make it clearer that it is a bad plan.
> 
> If it is okay to remove an option, we should not talk about it at all
> anymore.  Well maybe warn about it for another release or so, but not
> longer.

OK, thanks for the suggestion.

> 
>>>>  (define_register_constraint "we" "rs6000_constraints[RS6000_CONSTRAINT_we]"
>>>> -  "@internal Like @code{wa}, if @option{-mpower9-vector} and @option{-m64} are
>>>> -   used; otherwise, @code{NO_REGS}.")
>>>> +  "@internal Like @code{wa}, if the cpu type is power9 or up, meanwhile
>>>> +   @option{-mvsx} and @option{-m64} are used; otherwise, @code{NO_REGS}.")
>>>
>>> "if this is a POWER9 or later and @option{-mvsx} and @option{-m64} are
>>> used".  How clumsy.  Maybe we should make the patterns that use "we"
>>> work without mtvsrdd as well?  Hrm, they will still require 64-bit GPRs
>>> of course, unless we can do something tricky.
>>>
>>> We do not need the special constraint at all of course (we can add these
>>> conditions to all patterns that use it: all *two* patterns).  So maybe
>>> that's what we should do :-)
>>
>> Not sure the original intention introducing it (Mike might know it best), but
>> removing it sounds doable.
> 
> It is for mtvsrdd.

Yes, I meant to say not sure if there was some obstacle which made us introduce
a new constraint, or just because it's simple.

> 
>>  btw, it seems more than two patterns using it?
>> like (if I didn't miss something):
>>   - vsx_concat_<mode>
>>   - vsx_splat_<mode>_reg
>>   - vsx_splat_v4si_di
>>   - vsx_mov<mode>_64bit
> 
> Yes, it isn't clear we should use this contraint in those last two.  It
> looks like those do not even need the restriction to 64 bit systems.
> Well the last one obviously has that already, but then it could just use
> "wa", no?

For vsx_splat_v4si_di, it's for mtvsrws, ISA notes GPR[RA].bit[32:63] which
implies the context has 64bit GPR?  The last one seems still to distinguish
there is power9 support or not, just use "wa" which only implies power7
doesn't fit with it?

btw, the actual guard for "we" is TARGET_POWERPC64 rather than TARGET_64BIT,
the documentation isn't accurate enough.  Just filed internal issue #1345
for further tracking on this.

> 
>>> -mcpu=power8 implies -mvsx (power7 already).  You can disable VSX, or
>>> VMX as well, but by default it is enabled.
>>
>> Yes, it's meant to consider the explicitly -mno-vsx, which suffers the option
>> order issue.  But considering we raise error for -mno-vsx -mpower{8,9}-vector
>> before, without specifying -mvsx is closer to the previous.
>>
>> I'll adjust it and the below similar ones, thanks!
> 
> It is never supported to do unsupported things :-)
> 
> We need to be able to rely on defaults.  Otherwise, we will have to
> implement all of GCC recursively, in itself, in the testsuite, and in
> individual tests.  Let's not :-)

OK, fair enough.  Thanks!

BR,
Kewen