Re: [PATCH] rs6000: Neuter option -mpower{8,9}-vector [PR109987]
on 2024/2/21 09:37, Peter Bergner wrote:
> On 2/20/24 3:27 AM, Kewen.Lin wrote:
>> on 2024/2/20 02:45, Segher Boessenkool wrote:
>>> On Tue, Jan 16, 2024 at 10:50:01AM +0800, Kewen.Lin wrote:
it consists of some aspects:
- effective target powerpc_p{8,9}vector_ok are removed
and replaced with powerpc_vsx_ok.
>>>
>>> So all such testcases already arrange to have p8 or p9 some other way?
>
> Shouldn't that be replaced with powerpc_vsx instead of powerpc_vsx_ok?
> That way we know VSX code gen is enabled for the options being used,
> even those in RUNTESTFLAGS.
>
> I thought we agreed that powerpc_vsx_ok was almost always useless and
> we always want to use powerpc_vsx. ...or did I miss that we removed
> the old powerpc_vsx_ok and renamed powerpc_vsx to powerpc_vsx_ok?
Yes, I think we all agreed that powerpc_vsx matches more with what we
expect, but I'm hesitating to make such change at this stage because:
1. if testing on an env without vsx support, the test results on these
affected test cases may change a lot, as many test cases would
become unsupported (they pass before with explicit -mvsx before).
2. teach current powerpc_vsx to make use of current_compiler_flags
just like some existing practices on has_arch_* may help mitigate
it as not few test cases already have explicit -mvsx. But AIUI
current_complier_flags requires dg-options line comes first before
the effective target line to make options in dg-options take
effect, it means we need some work to adjust line order for the
affected test cases. On the other hand, some enhancement is needed
for current_compiler_flags as powerpc_vsx (old powerpc_vsx_ok) isn't
only used in test case but can be also used in some exp check
where no expected flags exist.
3. there may be some other similar effective target checks which we
want to update as well, it means we need a re-visit on the existing
effective target checks (rs6000 specific).
4. powerpc_vsx_ok has been there for a long long time, and -mno-vsx
is rarely used in RUNTESTFLAGS, this only affects testing, so it
is not that urgent.
so I'm inclined to work on this in next stage 1. What do you think?
>
- Some test cases are updated with explicit -mvsx.
- Some test cases with those two option mixed are adjusted
to keep the test points, like -mpower8-vector
-mno-power9-vector are updated with -mdejagnu-cpu=power8
-mvsx etc.
>>>
>>> -mcpu=power8 implies -mvsx already.
>
> Then we can omit the explicit -msx option, correct? Ie, if the
> user forces -mno-vsx in RUNTESTFLAGS, then we'll just skip the
> test case as UNSUPPORTED rather than trying to compile some
> vsx test case with vsx disabled via the options.
Yes, we can strip any -mvsx then, but if we want the test case
to be tested when it's able to, we can still append an extra
-mvsx. Even if -mno-vsx is specified but if the option order
makes it like "-mno-vsx... -mvsx", powerpc_vsx is supported
so that the test case can be still tested well with -mvsx
enabled, while if the order is like "-mvsx ... -mno-vsx",
powerpc_vsx fails and it becomes unsupported.
BR,
Kewen
Re: [PATCH] rs6000: Neuter option -mpower{8,9}-vector [PR109987]
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_
>> - vsx_splat__reg
>> - vsx_splat_v4si_di
>> - vsx_mov_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
Re: [PATCH] rs6000: Neuter option -mpower{8,9}-vector [PR109987]
On 2/20/24 3:27 AM, Kewen.Lin wrote:
> on 2024/2/20 02:45, Segher Boessenkool wrote:
>> On Tue, Jan 16, 2024 at 10:50:01AM +0800, Kewen.Lin wrote:
>>> it consists of some aspects:
>>> - effective target powerpc_p{8,9}vector_ok are removed
>>> and replaced with powerpc_vsx_ok.
>>
>> So all such testcases already arrange to have p8 or p9 some other way?
Shouldn't that be replaced with powerpc_vsx instead of powerpc_vsx_ok?
That way we know VSX code gen is enabled for the options being used,
even those in RUNTESTFLAGS.
I thought we agreed that powerpc_vsx_ok was almost always useless and
we always want to use powerpc_vsx. ...or did I miss that we removed
the old powerpc_vsx_ok and renamed powerpc_vsx to powerpc_vsx_ok?
>>> - Some test cases are updated with explicit -mvsx.
>>> - Some test cases with those two option mixed are adjusted
>>> to keep the test points, like -mpower8-vector
>>> -mno-power9-vector are updated with -mdejagnu-cpu=power8
>>> -mvsx etc.
>>
>> -mcpu=power8 implies -mvsx already.
Then we can omit the explicit -msx option, correct? Ie, if the
user forces -mno-vsx in RUNTESTFLAGS, then we'll just skip the
test case as UNSUPPORTED rather than trying to compile some
vsx test case with vsx disabled via the options.
Peter
Re: [PATCH] rs6000: Neuter option -mpower{8,9}-vector [PR109987]
On Tue, Feb 20, 2024 at 05:27:07PM +0800, Kewen.Lin wrote:
> > -mcpu=power8 implies -mvsx already.
>
> Yes, but users can specify -mno-vsx in RUNTESTFLAGS, dejagnu
> framework can have different behaviors (options order) for
> different versions, this explicit -mvsx is mainly for the
> consistency between the checking and the actual testing.
It is not supported at all. It is better to assume users do not try
to hang themselves.
> > It is mostly a testsuite patch, and testcase patches are fine (and much
> > wanted!) in stage 4. The actual compiler options remain, and behaviour
> > does not change for anyone who used the option as intended,
>
> Yes, excepting for one unexpected use that users having one cpu type which
> doesn't support power8/power9 capability but meanwhile specifies option
> -mpower{8,9}-vector to gain power8/power9 capability (as currently these
> options can enable the corresponding flags). But I don't think it's an
> expected use case.
Yeah, it is why we do not want such options at all :-)
> >>* config/rs6000/rs6000.opt: Make option power{8,9}-vector as
> >>WarnRemoved.
> >
> > Do we want this, or do we want it silent? Should we remove the options
> > later, if we now warn for it?
>
> 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.
> >> (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.
> btw, it seems more than two patterns using it?
> like (if I didn't miss something):
> - vsx_concat_
> - vsx_splat__reg
> - vsx_splat_v4si_di
> - vsx_mov_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?
> > -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 :-)
Cheers,
Segher
Re: [PATCH] rs6000: Neuter option -mpower{8,9}-vector [PR109987]
Hi Segher,
Thanks for the review comments!
on 2024/2/20 02:45, Segher Boessenkool wrote:
> Hi!
>
> On Tue, Jan 16, 2024 at 10:50:01AM +0800, Kewen.Lin wrote:
>> As PR109987 and its duplicated bugs show, -mno-power8-vector
>> (and -mno-power9-vector) cause some problems and as Segher
>> pointed out in [1] they are workaround options, so this patch
>> is to remove -m{no,}-power{8,9}-options.
>
> Excellent :-)
>
>> Like what we did
>> for option -mdirect-move before, this patch still keep the
>> corresponding internal flags and they are automatically set
>> based on -mcpu.
>
> Yup. That makes the code nicer, and it what we already have anyway!
>
>> The test suite update takes some efforts,
>
> Yeah :-/
>
>> it consists of some aspects:
>> - effective target powerpc_p{8,9}vector_ok are removed
>> and replaced with powerpc_vsx_ok.
>
> So all such testcases already arrange to have p8 or p9 some other way?
Some of them already have, but some of them don't, for those
without any p8/p9 are adjusted according to the test points
as below explanation.
>
>> - Some cases having -mpower{8,9}-vector are updated with
>> -mvsx, some of them already have -mdejagnu-cpu. For
>> those that don't have -mdejagnu-cpu, if -mdejagnu-cpu
>> is needed for the test point, then it's appended;
>> otherwise, add additional-options -mdejagnu-cpu=power{8,9}
>> if has_arch_pwr{8,9} isn't satisfied.
>
> Yeah it's a judgement call every time.
>
>> - Some test cases are updated with explicit -mvsx.
>> - Some test cases with those two option mixed are adjusted
>> to keep the test points, like -mpower8-vector
>> -mno-power9-vector are updated with -mdejagnu-cpu=power8
>> -mvsx etc.
>
> -mcpu=power8 implies -mvsx already.
Yes, but users can specify -mno-vsx in RUNTESTFLAGS, dejagnu
framework can have different behaviors (options order) for
different versions, this explicit -mvsx is mainly for the
consistency between the checking and the actual testing.
But according to the discussion in an internal thread, the
current powerpc_vsx_ok doesn't work as what we expect, there
will be some changes later.
>
>> - Some test cases with -mno-power{8,9}-vector are updated
>> by replacing -mno-power{8,9}-vector with -mno-vsx, or
>> just removing it.
>
> Okay.
>
>> - For some cases, we don't always specify -mdejagnu-cpu to
>> avoid to restrict the testing coverage, it would check
>> has_arch_pwr{8,9} and appended that as need.
>
> That is in general how all tests should be. Very sometimes we want to
> test for a specific CPU, for a regression test that exhibited just on a
> certain CPU for example. But we should never have a -mcpu= (or a
> -mpowerN-vector nastiness thing) to test things on a new CPU! Just do a
> testsuite ruyn with *that* CPU. Not many years from now, *all* CPUs
> will have those new instructions anyway, so let's not put noise in the
> testcases that will be irrelevant soon.
>
>> - For vect test cases run, it doesn't specify -mcpu=power9
>> for power10 and up.
>>
>> Bootstrapped and regtested on:
>> - powerpc64-linux-gnu P7/P8/P9 {-m32,-m64}
>> - powerpc64le-linux-gnu P8/P9/P10
>
> In general it is nice to test 970 as the lowest vector thing we have,
> abnd/or p4 as a target without anything vector, as well. But I expect
> thoise will just work for this patch :-)
Thanks for the tips, I'll give them a shot before pushing it.
>
>> Although it's stage4 now, as the discussion in PR113115 we
>> are still eager to neuter these two options.
>
> It is mostly a testsuite patch, and testcase patches are fine (and much
> wanted!) in stage 4. The actual compiler options remain, and behaviour
> does not change for anyone who used the option as intended,
Yes, excepting for one unexpected use that users having one cpu type which
doesn't support power8/power9 capability but meanwhile specifies option
-mpower{8,9}-vector to gain power8/power9 capability (as currently these
options can enable the corresponding flags). But I don't think it's an
expected use case.
>
> Okay for trunk. Thanks! Comments below:
>
>> * config/rs6000/rs6000.opt: Make option power{8,9}-vector as
>> WarnRemoved.
>
> Do we want this, or do we want it silent? Should we remove the options
> later, if we now warn for it?
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?
>
>> (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;
Re: [PATCH] rs6000: Neuter option -mpower{8,9}-vector [PR109987]
Hi!
On Tue, Jan 16, 2024 at 10:50:01AM +0800, Kewen.Lin wrote:
> As PR109987 and its duplicated bugs show, -mno-power8-vector
> (and -mno-power9-vector) cause some problems and as Segher
> pointed out in [1] they are workaround options, so this patch
> is to remove -m{no,}-power{8,9}-options.
Excellent :-)
> Like what we did
> for option -mdirect-move before, this patch still keep the
> corresponding internal flags and they are automatically set
> based on -mcpu.
Yup. That makes the code nicer, and it what we already have anyway!
> The test suite update takes some efforts,
Yeah :-/
> it consists of some aspects:
> - effective target powerpc_p{8,9}vector_ok are removed
> and replaced with powerpc_vsx_ok.
So all such testcases already arrange to have p8 or p9 some other way?
> - Some cases having -mpower{8,9}-vector are updated with
> -mvsx, some of them already have -mdejagnu-cpu. For
> those that don't have -mdejagnu-cpu, if -mdejagnu-cpu
> is needed for the test point, then it's appended;
> otherwise, add additional-options -mdejagnu-cpu=power{8,9}
> if has_arch_pwr{8,9} isn't satisfied.
Yeah it's a judgement call every time.
> - Some test cases are updated with explicit -mvsx.
> - Some test cases with those two option mixed are adjusted
> to keep the test points, like -mpower8-vector
> -mno-power9-vector are updated with -mdejagnu-cpu=power8
> -mvsx etc.
-mcpu=power8 implies -mvsx already.
> - Some test cases with -mno-power{8,9}-vector are updated
> by replacing -mno-power{8,9}-vector with -mno-vsx, or
> just removing it.
Okay.
> - For some cases, we don't always specify -mdejagnu-cpu to
> avoid to restrict the testing coverage, it would check
> has_arch_pwr{8,9} and appended that as need.
That is in general how all tests should be. Very sometimes we want to
test for a specific CPU, for a regression test that exhibited just on a
certain CPU for example. But we should never have a -mcpu= (or a
-mpowerN-vector nastiness thing) to test things on a new CPU! Just do a
testsuite ruyn with *that* CPU. Not many years from now, *all* CPUs
will have those new instructions anyway, so let's not put noise in the
testcases that will be irrelevant soon.
> - For vect test cases run, it doesn't specify -mcpu=power9
> for power10 and up.
>
> Bootstrapped and regtested on:
> - powerpc64-linux-gnu P7/P8/P9 {-m32,-m64}
> - powerpc64le-linux-gnu P8/P9/P10
In general it is nice to test 970 as the lowest vector thing we have,
abnd/or p4 as a target without anything vector, as well. But I expect
thoise will just work for this patch :-)
> Although it's stage4 now, as the discussion in PR113115 we
> are still eager to neuter these two options.
It is mostly a testsuite patch, and testcase patches are fine (and much
wanted!) in stage 4. The actual compiler options remain, and behaviour
does not change for anyone who used the option as intended,
Okay for trunk. Thanks! Comments below:
> * config/rs6000/rs6000.opt: Make option power{8,9}-vector as
> WarnRemoved.
Do we want this, or do we want it silent? Should we remove the options
later, if we now warn for it?
> (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 :-)
> -If you use the ISA 3.0 instruction set (@option{-mpower9-vector} or
> -@option{-mcpu=power9}) on a 64-bit system, the IEEE 128-bit floating
> -point support will also enable the generation of ISA 3.0 IEEE 128-bit
> -floating point instructions. Otherwise, if you do not specify to
> -generate ISA 3.0 instructions or you are targeting a 32-bit big endian
> -system, IEEE 128-bit floating point will be done with software
> -emulation.
> +If you use the ISA 3.0 instruction set (@option{-mcpu=power9}) on a
> +64-bit system, the IEEE 128-bit floating point support will also enable
> +the generation of ISA 3.0 IEEE 128-bit floating point instructions.
> +Otherwise, if you do not specify to generate ISA 3.0 instructions or you
> +are targeting a 32-bit big endian system, IEEE 128-bit floating point
> +will be done with software emulation.
You do not need to reformat documentation source code: it is
automatically formatted in all output formats and all viewers :-)
> diff --git
