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
@@ -2010,12 +2010,13 @@
XXSPLTD_V2DI vsx_xxspltd_v2di {}
; Power7 builtins (ISA 2.06).
[power7]
- const unsigned int __builtin_addg6s (unsigned int, unsigned int);
- ADDG6S addg6s {}
+
+ const unsigned int __builtin_addg6s_32 (unsigned int, unsigned int);
+ ADDG6S_32 addg6ssi3 {}
const signed long __builtin_bpermd (signed long, signed long);
BPERMD bpermd_di {32bit}
const unsigned int __builtin_cbcdtd (unsigned int);
@@ -2041,10 +2042,14 @@
UNPACK_V1TI unpackv1ti {}
; Power7 builtins requiring 64-bit GPRs (even with 32-bit addressing).
[power7-64]
+
+ const unsigned long __builtin_addg6s_64 (unsigned long, unsigned long);
+ ADDG6S addg6sdi3 {no32bit}
+
const signed long long __builtin_divde (signed long long, signed long long);
DIVDE dive_di {}
const unsigned long long __builtin_divdeu (unsigned long long, \
unsigned long long);
@@ -1919,10 +1919,35 @@ altivec_resolve_overloaded_builtin (location_t loc, tree fndecl,
instance_code, fcode, types, args);
if (call != error_mark_node)
return call;
break;
}
+ /* We need to special case __builtin_addg6s because the overloaded
+ forms of this function take (unsigned int, unsigned int) or
+ (unsigned long long, unsigned long long). Since C conventions
+ allow the respective argument types to be implicitly coerced into
+ each other, the default handling does not provide adequate
+ discrimination between the desired forms of the function. */
+ case RS6000_OVLD_ADDG6S_OV:
+ {
+ machine_mode arg1_mode = TYPE_MODE (types[0]);
+ machine_mode arg2_mode = TYPE_MODE (types[1]);
+
+ /* If any supplied arguments are wider than 32 bits, resolve to
+ 64-bit variant of built-in function. */
+ if (GET_MODE_PRECISION (arg1_mode) > 32
+ || GET_MODE_PRECISION (arg2_mode) > 32)
+ instance_code = RS6000_BIF_ADDG6S;
+ else
+ instance_code = RS6000_BIF_ADDG6S_32;
+
+ tree call = find_instance (&unsupported_builtin, &instance,
+ instance_code, fcode, types, args);
+ if (call != error_mark_node)
+ return call;
+ break;
+ }
case RS6000_OVLD_VEC_VSIE:
{
machine_mode arg1_mode = TYPE_MODE (types[0]);
/* If supplied first argument is wider than 64 bits, resolve to
@@ -193,10 +193,16 @@
unsigned int __builtin_cmpb (unsigned int, unsigned int);
CMPB_32
unsigned long long __builtin_cmpb (unsigned long long, unsigned long long);
CMPB
+[ADDG6S_OV, SKIP, __builtin_addg6s]
+ unsigned int __builtin_addg6s (unsigned int, unsigned int);
+ ADDG6S_32
+ unsigned long long __builtin_addg6s (unsigned long long, unsigned long long);
+ ADDG6S
+
[VEC_ABS, vec_abs, __builtin_vec_abs]
vsc __builtin_vec_abs (vsc);
ABS_V16QI
vss __builtin_vec_abs (vss);
ABS_V8HI
@@ -14534,14 +14534,14 @@ (define_peephole2
operands[5] = change_address (mem, <ALTIVEC_DFORM:MODE>mode, new_addr);
})
;; Miscellaneous ISA 2.06 (power7) instructions
-(define_insn "addg6s"
- [(set (match_operand:SI 0 "register_operand" "=r")
- (unspec:SI [(match_operand:SI 1 "register_operand" "r")
- (match_operand:SI 2 "register_operand" "r")]
+(define_insn "addg6s<mode>3"
+ [(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")])
@@ -18139,10 +18139,11 @@ addition to the @option{-maltivec}, @option{-mpopcntd}, and
@option{-mvsx} options.
The following basic built-in functions require @option{-mpopcntd}:
@smallexample
unsigned int __builtin_addg6s (unsigned int, unsigned int);
+unsigned long long __builtin_addg6s (unsigned long long, unsigned long long);
long long __builtin_bpermd (long long, long long);
unsigned int __builtin_cbcdtd (unsigned int);
unsigned int __builtin_cdtbcd (unsigned int);
long long __builtin_divde (long long, long long);
unsigned long long __builtin_divdeu (unsigned long long, unsigned long long);
new file mode 100644
@@ -0,0 +1,30 @@
+/* { dg-do compile { target { powerpc*-*-linux* } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } } */
+/* { dg-require-effective-target powerpc_vsx_ok } */
+/* { dg-options "-mdejagnu-cpu=power7 -O3" } */
+/* { dg-final { scan-assembler-times {\maddg6s\M} 4 } } */
+/* { dg-final { scan-assembler-not "bl __builtin" } } */
+
+/* Test case for the addg6s builtin, exercising both
+ unsigned int and unsigned long long arguments. */
+
+unsigned long long test2_ull (unsigned long long a, unsigned long long b)
+{
+ return __builtin_addg6s (a, b);
+}
+
+unsigned int test1_ui (unsigned int a, unsigned int b)
+{
+ return __builtin_addg6s (a, b);
+}
+
+unsigned int test3_ui (unsigned int *a, unsigned int *b)
+{
+ return __builtin_addg6s(*a, *b);
+}
+
+unsigned long long test4_ull (unsigned long long *a, unsigned long long *b)
+{
+ return __builtin_addg6s(*a, *b);
+}
+
new file mode 100644
@@ -0,0 +1,78 @@
+/* { dg-do run { target { powerpc*-*-linux* } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } } */
+/* { dg-require-effective-target powerpc_vsx_ok } */
+/* { dg-options "-mdejagnu-cpu=power7 -O3" } */
+
+/* Test case for the addg6s builtin, exercising both
+ unsigned int and unsigned long long arguments. */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+unsigned int test_addg6s_int (unsigned int a, unsigned int b)
+{
+ return __builtin_addg6s (a, b);
+}
+
+unsigned long long test_addg6s_longlong (unsigned long long a, unsigned long long b)
+{
+ return __builtin_addg6s (a, b);
+}
+
+/* Table of expected values. */
+
+unsigned int exp_i[] = {
+0x66666660,
+0x66666606,
+0x66666066,
+0x66660666,
+0x66606666,
+0x66066666,
+0x60666666,
+0x06666666,
+0x77777777
+};
+
+unsigned long long exp_ll[] = {
+0x6666666666666660,
+0x6666666666666606,
+0x6666666666666066,
+0x6666666666660666,
+0x6666666666606666,
+0x6666666666066666,
+0x6666666660666666,
+0x6666666606666666,
+0x6666666066666666,
+0x6666660666666666,
+0x6666606666666666,
+0x6666066666666666,
+0x6660666666666666,
+0x6606666666666666,
+0x6066666666666666,
+0x0666666666666666,
+0x7777777777777777
+};
+
+int main() {
+ unsigned int intresult;
+ unsigned long long longresult;
+ int idx;
+ int fail=0;
+
+ for (idx=0; idx<8; idx++) {
+ intresult = test_addg6s_int (0x01<<4*idx, 0x0f<<4*idx);
+ if (exp_i[idx] != intresult)
+ printf("index:%2d Got:%8x Expected:%8x %d\n",
+ idx, intresult, exp_i[idx], ++fail);
+ }
+
+ for (idx=0; idx<16; idx++) {
+ longresult = test_addg6s_longlong (0x01ULL<<4*idx, 0x0fULL<<4*idx);
+ if (exp_ll[idx] != longresult)
+ printf("index:%2d Got:%16llx Expected:%16llx %d\n",
+ idx, longresult, exp_ll[idx], ++fail);
+ }
+ if (fail)
+ abort();
+}
+