Re: [PATCH, rs6000] Fix addg6s builtin with long long parameters. (PR100693)

[Segher Boessenkool <segher@kernel.crashing.org>]

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
> 	("addg6s<mode>3") and rework.

	* config/rs6000/rs6000.md (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/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