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

[Segher Boessenkool <segher@kernel.crashing.org>]

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 a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
> index 81ae92a0266..df56a86cf05 100644
> --- a/gcc/testsuite/lib/target-supports.exp
> +++ b/gcc/testsuite/lib/target-supports.exp
> @@ -2621,7 +2621,7 @@ proc check_p8vector_hw_available { } {
>  	     || [istarget *-*-darwin*]} {
>  	    expr 0
>  	} else {
> -	    set options "-mpower8-vector"
> +	    set options "-mcpu=power8 -mvsx"

-mcpu=power8 implies -mvsx (power7 already).  You can disable VSX, or
VMX as well, but by default it is enabled.

> @@ -2649,7 +2649,7 @@ proc check_p9vector_hw_available { } {
>  	     || [istarget *-*-darwin*]} {
>  	    expr 0
>  	} else {
> -	    set options "-mpower9-vector"
> +	    set options "-mcpu=power9 -mvsx"

Same here.

> @@ -11659,9 +11603,14 @@ proc check_vect_support_and_set_flags { } {
> 
>          lappend DEFAULT_VECTCFLAGS "-maltivec"
>          if [check_p9vector_hw_available] {
> -            lappend DEFAULT_VECTCFLAGS "-mpower9-vector"
> +            # For power10 and up, don't specify -mcpu=power9, then we
> +            # can have more testing coverage with higher cpu types.

s/then/so that/

> +            lappend DEFAULT_VECTCFLAGS "-mvsx"
>          } elseif [check_p8vector_hw_available] {
> -            lappend DEFAULT_VECTCFLAGS "-mpower8-vector"
> +            lappend DEFAULT_VECTCFLAGS "-mcpu=power8" "-mvsx"

No need for -mvsx .

> --- a/libgcc/configure
> +++ b/libgcc/configure
> @@ -5238,7 +5238,7 @@ fi
>  { $as_echo "$as_me:${as_lineno-$LINENO}: result: $libgcc_cv_powerpc_float128" >&5
>  $as_echo "$libgcc_cv_powerpc_float128" >&6; }
> 
> -  CFLAGS="$CFLAGS -mpower9-vector -mfloat128-hardware"
> +  CFLAGS="$CFLAGS -mcpu=power9 -mvsx -mfloat128-hardware"

Same.

I did not check all testcases to see whether you made a good tradeoff
in this conversion, it does look good on a cursory view.

So just a few tweaks to the English, and it is good to go :-)  Thanks
again!


Segher