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 9555B3858CDA for ; Fri, 7 Oct 2022 20:01:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9555B3858CDA 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 297K0CGc025717; Fri, 7 Oct 2022 15:00:12 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 297K0B3v025716; Fri, 7 Oct 2022 15:00:11 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Fri, 7 Oct 2022 15:00:10 -0500 From: Segher Boessenkool To: will schmidt Cc: "'GCC Patches'" , "Kewen.Lin" , David Edelsohn Subject: Re: [PATCH, rs6000] Fix addg6s builtin with long long parameters. (PR100693) Message-ID: <20221007200010.GZ25951@gate.crashing.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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,KAM_SHORT,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: Hi! On Thu, Oct 06, 2022 at 04:29:57PM -0500, will schmidt wrote: > As reported in PR 100693, attempts to use __builtin_addg6s > with long long arguments result in truncated results. > > Since the int and long long types can be coerced into each other, > (documented further near the rs6000-c.cc change), this is handled > by adding a builtin overload (ADDG6S_OV), and the addition of some > special handling in altivec_resolve_overloaded_builtin() to map > the calls to addg6s_32 or addg6s_64; similar to how the SCAL_CMPB > builtins are currently handled. Another option is to make "addg6sl" and "addg6ll" versions, like many generic builtins have (popcount for example). This is ugly and not very userfriendly of course. OTOH, the overload thing is more confusing, if you use constant arguments for example. Have you considered just always using "long" arguments, treating this as a bugfix? It will only hurt users that depend on 64-bit arguments being cut short to 32 bits (implicitly!), not a very sensible thing to expect. > I'm seeing a regression failure show up in > testsuite/g++.dg/modules/adl-3*.c; which seems entirely unrelated > to the content in this change. I'm poking at that a bit more to > see if I can tell the what/why for that. Yeah, we need that resolved before this patch can be okay at all. But I guess it is just an unstable test? > * config/rs6000/rs6000-builtins.def ([POWER7]): Replace bif-name > __builtin_addg6s with bif-name __builtin_addg6s_32. The stanza is lowercase, not uppercase. But, Sthis should be * config/rs6000/rs6000-builtins.def (ADDG6S): Replace bif-name __builtin_addg6s with bif-name __builtin_addg6s_32. or don't even mention the old name? Like * config/rs6000/rs6000-builtins.def (ADDG6S): Use bif-name __builtin_addg6s_32. > ([POWER7-64]): New bif-name __builtin_addg6s_64. Similar. > * config/rs6000/rs6000-c.cc (altivec_resolve_overloaded_builtin): > Add handler mapping RS6000_OVLD_ADDG6S_OV to RS6000_BIF_ADDG6S > and RS6000_BIF_ADDG6S_32. Please don't wrap lines early, certainly not if that then leaves a colon at the end of a line (it looks like you forgot something, that way). Changelog lines are 80 character positions long. > * config/rs6000/rs6000.md ("addg6s", UNSPEC_ADDG6S): Replace with > ("addg6s3") and rework. * config/rs6000/rs6000.md (addg6s): Delete. (addg6s3 for GPR): New. There is no pattern called "UNSPEC_ADDG6S", it doesn't belong in the changelog :-) > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr100693-compile.c > @@ -0,0 +1,30 @@ > +/* { dg-do compile { target { powerpc*-*-linux* } } } */ target {*-*-linux*} (everything is powerpc*-*-* in gcc.target/powerpc) > +/* { dg-skip-if "" { powerpc*-*-darwin* } } */ Don't skip on darwin unless you know it is needed. > +/* { dg-require-effective-target powerpc_vsx_ok } */ addg6s does not need VSX. You want has_arch_pwr7 here? > +/* { dg-options "-mdejagnu-cpu=power7 -O3" } */ -O2 if that works please. > +/* { dg-final { scan-assembler-times {\maddg6s\M} 4 } } */ > +/* { dg-final { scan-assembler-not "bl __builtin" } } */ Some ABIs will use tailcalls here. You can prevent that in the testcase (add an asm(""); before the return statement), or you can scan for something less specific than the exact "bl"? > +unsigned long long test4_ull (unsigned long long *a, unsigned long long *b) > +{ > + return __builtin_addg6s(*a, *b); > +} This does not work with -m32, no? Segher