From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id 244163858D32 for ; Mon, 19 Feb 2024 18:46:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 244163858D32 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kernel.crashing.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 244163858D32 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=63.228.1.57 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708368388; cv=none; b=fjWNUV8BJgkwZrrliW/U7ykcvwo06MziZy2KwbrjcA7I8fdIEfFC3J8LXhEvHvp8odD2DtBo6YWR4aX62egCarOnqNbZvIsWk38QlOd5BiU1r/UczFGaShOa+0beSUC7pJ9g89nBKx19arSaCG1gewDBnM/xeqJGEICrxz5ELaM= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708368388; c=relaxed/simple; bh=rRkylCY+MAQrwe6JUMAUo6wj9srQcrC4/kFGHPJSMoo=; h=Date:From:To:Subject:Message-ID:Mime-Version; b=lc5qhJMP0+wzr5XZjHNAdWmvdTe4P6eL6yz0ZOIj4tDaXUnO5UKMP3nIRK+xgc2HYHUVMPmOIM8cTcSAPkicQ2LLwpPCpF8G4dX9BaM08y7jyYCv73b2shVNxAgdKfJ/X+ISsv6f3ZKUyzx+jswxQVxmnyzKvk3MFEUtIMhQjVA= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 41JIjPso020547; Mon, 19 Feb 2024 12:45:25 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 41JIjO0C020546; Mon, 19 Feb 2024 12:45:24 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Mon, 19 Feb 2024 12:45:24 -0600 From: Segher Boessenkool To: "Kewen.Lin" Cc: GCC Patches , Peter Bergner , David Edelsohn , Michael Meissner Subject: Re: [PATCH] rs6000: Neuter option -mpower{8,9}-vector [PR109987] Message-ID: <20240219184524.GH19790@gate.crashing.org> References: <05d9d61e-af45-908c-c509-4b81073c4486@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <05d9d61e-af45-908c-c509-4b81073c4486@linux.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00,GIT_PATCH_0,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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