public inbox for
 help / color / mirror / Atom feed
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)
Date: Fri, 7 Oct 2022 15:00:10 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>


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 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

> 	([POWER7-64]): New bif-name __builtin_addg6s_64.


> 	* config/rs6000/ (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/ ("addg6s", UNSPEC_ADDG6S): Replace with
> 	("addg6s<mode>3") and rework.

	* config/rs6000/ (addg6s): Delete.
	(addg6s<mode>3 for GPR): New.

There is no pattern called "UNSPEC_ADDG6S", it doesn't belong in the
changelog :-)

> --- /dev/null
> +++ b/gcc/testsuite/
> @@ -0,0 +1,30 @@
> +/* { dg-do compile { target { powerpc*-*-linux* } } } */

target {*-*-linux*}  (everything is powerpc*-*-* in

> +/* { 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?


      reply	other threads:[~2022-10-07 20:01 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-06 21:29 will schmidt
2022-10-07 20:00 ` Segher Boessenkool [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).