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 926123857C50 for ; Wed, 16 Mar 2022 18:13:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 926123857C50 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 22GICoST028215; Wed, 16 Mar 2022 13:12:50 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 22GICoKf028214; Wed, 16 Mar 2022 13:12:50 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Wed, 16 Mar 2022 13:12:49 -0500 From: Segher Boessenkool To: will schmidt Cc: gcc-patches@gcc.gnu.org, David Edelsohn , Peter Bergner Subject: Re: rs6000: RFC/Update support for addg6s instruction. PR100693 Message-ID: <20220316181249.GK614@gate.crashing.org> References: <3b4976974ca4a9e481c462ef2b9a4892f1d4174f.camel@vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3b4976974ca4a9e481c462ef2b9a4892f1d4174f.camel@vnet.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, KAM_NUMSUBJECT, KAM_SHORT, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Mar 2022 18:13:53 -0000 Hi! On Wed, Mar 16, 2022 at 12:20:18PM -0500, will schmidt wrote: > For PR100693, we currently provide an addg6s builtin using unsigned > int arguments, but we are missing an unsigned long long argument > equivalent. This patch adds an overload to provide the long long > version of the builtin. > > unsigned long long __builtin_addg6s (unsigned long long, unsigned long long); > > RFC/concerns: This patch works, but looking briefly at intermediate stages > is not behaving quite as I expected. Looking at the intermediate dumps, I > see in pr100693.original that calls I expect to be routed to the internal > __builtin_addg6s_si() that uses (unsigned int) arguments are instead being > handled by __builtin_addg6s_di() with casts that convert the arguments to > (unsigned long long). Did you test with actual 32-bit variables, instead of just function arguments? Function arguments are always passed in (sign-extended) registers. Like, unsigned int f(unsigned int *a, unsigned int *b) { return __builtin_addg6s(*a, *b); } > As a test, I see if I swap the order of the builtins in rs6000-overload.def > I end up with code casting the ULL values to UI, which provides truncated > results, and is similar to what occurs today without this patch. > > All that said, this patch seems to work. OK for next stage 1? > Tested on power8BE as well as LE power8,power9,power10. Please ask again when stage 1 has started? > gcc/ > PR target/100693 > * config/rs6000/rs600-builtins.def: Remove entry for __builtin_addgs() > and add entries for __builtin_addg6s_di() and __builtin_addg6s_si(). Indent of second and further lines should be at the "*", not two spaces after that. > - UNSPEC_ADDG6S > + UNSPEC_ADDG6S_SI > + UNSPEC_ADDG6S_DI You do not need multiple unspec numbers. You can differentiate them based on the modes of the arguments, already :-) > ;; Miscellaneous ISA 2.06 (power7) instructions > -(define_insn "addg6s" > +(define_insn "addg6s_si" > [(set (match_operand:SI 0 "register_operand" "=r") > (unspec:SI [(match_operand:SI 1 "register_operand" "r") > (match_operand:SI 2 "register_operand" "r")] > - UNSPEC_ADDG6S))] > + UNSPEC_ADDG6S_SI))] > + "TARGET_POPCNTD" > + "addg6s %0,%1,%2" > + [(set_attr "type" "integer")]) > + > +(define_insn "addg6s_di" > + [(set (match_operand:DI 0 "register_operand" "=r") > + (unspec:DI [(match_operand:DI 1 "register_operand" "r") > + (match_operand:DI 2 "register_operand" "r")] > + UNSPEC_ADDG6S_DI))] > "TARGET_POPCNTD" > "addg6s %0,%1,%2" > [(set_attr "type" "integer")]) (define_insn "addg6s" [(set (match_operand:GPR 0 "register_operand" "=r") (unspec:GPR [(match_operand:GPR 1 "register_operand" "r") (match_operand:GPR 2 "register_operand" "r")] UNSPEC_ADDG6S))] "TARGET_POPCNTD" "addg6s %0,%1,%2" [(set_attr "type" "integer")]) We do not want DI (here, and in most places) for -m32! > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr100693.c > @@ -0,0 +1,68 @@ > +/* { dg-do compile { target { powerpc*-*-linux* } } } */ Why only on Linux? > +/* { dg-skip-if "" { powerpc*-*-darwin* } } */ Why not on Darwin? And why skip it anyway, given the previous line :-) > +/* { dg-require-effective-target powerpc_vsx_ok } */ That is the wrong requirement. You want to test for Power7, not for VSX. I realise you probably copied this from elsewhere :-( (If from another addg6s testcase, just keep it). Segher