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 C2E9D3858290 for ; Mon, 17 Oct 2022 18:09:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C2E9D3858290 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 Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 29HI8fkd002631; Mon, 17 Oct 2022 13:08:42 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 29HI8eoC002630; Mon, 17 Oct 2022 13:08:40 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Mon, 17 Oct 2022 13:08:40 -0500 From: Segher Boessenkool To: will schmidt Cc: GCC patches , David Edelsohn , "Kewen.Lin" , Michael Meissner Subject: Re: [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865] (2/2) Message-ID: <20221017180840.GJ25951@gate.crashing.org> References: <2656f0182df289ade33411ac579d2d5a229f5e4c.camel@vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2656f0182df289ade33411ac579d2d5a229f5e4c.camel@vnet.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Mon, Sep 19, 2022 at 11:13:20AM -0500, will schmidt wrote: > The _ARCH_PWR8 define is conditional on TARGET_DIRECT_MOVE, > and can be disabled by dependent options when it should not be. > This manifests in the issue seen in PR101865 where -mno-vsx > mistakenly disables _ARCH_PWR8. > This change replaces the relevant TARGET_DIRECT_MOVE references > with a TARGET_POWER8 entry so that the direct_move and power8 > features can be enabled or disabled independently. We should get rid of TARGET_DIRECT_MOVE altogether. Please see 57f108f5a1e1: rs6000: Disable -m[no-]direct-move (PR85293) The -mno-direct-move option causes a lot of problems, since it forces us to be able to generate code for p8 and up with some crucial instructions missing. This patch removes the -m[no-]direct-move options so that the user cannot put us into this unexpected situation anymore. Internally we still have all the same flags, and they are automatically set based on -mcpu; getting rid of that is a lot more work and will have to wait for GCC 9 (in some places the flag is used to see if we are compiling for a p8 _at all_). It did not happen in GCC 9 obviously. Do you want to take a shot? It doesn't have to be all at once, it's probably best if not even -- as I wrote in the commit message, the flag always was used to mean different things. > The existing (and rather lengthy) commentary for DIRECT_MOVE remains > in place in rs6000-c.cc:rs6000_target_modify_macros(). The > if-defined logic there will now set a __DIRECT_MOVE__ define when > TARGET_DIRECT_MOVE is set, this serves as a placeholder for debug > purposes, but is otherwise unused. This can be removed in a > subsequent patch, or in an update of this patch, depending on feedback. There should be no such macro, for the same reason there should be no -mdirect-move option: it is so very essential to all code we generate, it *always* is enabled if we have P8 or later. > gcc/ > PR Target/101865 > * config/rs6000/rs6000-builtin.cc > (rs6000_builtin_is_supported): Replace TARGET_DIRECT_MOVE > usage with TARGET_POWER8. Please don't arbitrarily wrap lines. It is harder to read, and it looks like something is missing. > * config/rs6000/rs6000-cpus.def (ISA_2_7_MASKS_SERVER): > Add OPTION_MASK_POWER8 entry. Especially in cases like this, where it looks like you forgot to write something after the colon. > @@ -24046,10 +24045,11 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] = > { "block-ops-vector-pair", OPTION_MASK_BLOCK_OPS_VECTOR_PAIR, > false, true }, > { "cmpb", OPTION_MASK_CMPB, false, true }, > { "crypto", OPTION_MASK_CRYPTO, false, true }, > { "direct-move", OPTION_MASK_DIRECT_MOVE, false, true }, > + { "power8", OPTION_MASK_POWER8, false, true }, Why would we want a #pragma power8 ? > --- a/gcc/config/rs6000/rs6000.opt > +++ b/gcc/config/rs6000/rs6000.opt > @@ -490,10 +490,15 @@ mcrypto > Target Mask(CRYPTO) Var(rs6000_isa_flags) > Use ISA 2.07 Category:Vector.AES and Category:Vector.SHA2 instructions. > > mdirect-move > Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved > +Enable direct move (ISA 2.07). It is undocumented and should remain that, except eventually we should remove it completely (but leave some stubs so that code in the wild keeps compiling). > +mpower8 > +Target Mask(POWER8) Var(rs6000_isa_flags) > +Use instructions added in ISA 2.07 (power8). There should not be such an option. It is set by -mcpu=power8 and later, but can never be enabled or disabled direfctly by the user. > --- a/gcc/config/rs6000/vsx.md > +++ b/gcc/config/rs6000/vsx.md > @@ -3407,11 +3407,11 @@ (define_insn "vsx_extract_" > if (element == VECTOR_ELEMENT_SCALAR_64BIT) > { > if (op0_regno == op1_regno) > return ASM_COMMENT_START " vec_extract to same register"; > > - else if (INT_REGNO_P (op0_regno) && TARGET_DIRECT_MOVE > + else if (INT_REGNO_P (op0_regno) && TARGET_POWER8 > && TARGET_POWERPC64) That fits on one line now. Thanks, Segher