Hi! On 2/10/22 2:50 PM, Segher Boessenkool wrote: > On Thu, Feb 10, 2022 at 12:22:28PM -0600, Bill Schmidt wrote: >> This is a backport from mainline 3f30f2d1dbb3228b8468b26239fe60c2974ce2ac. >> These built-ins were misimplemented as always having big-endian semantics. >> >> Because the built-in infrastructure has changed, the modifications to the >> source are different but achieve the same purpose. The modifications to >> the test suite are identical (after fixing the issue with -mbig that David >> pointed out with the original patch). >> /* 1 argument vector functions added in ISA 3.0 (power9). */ >> -BU_P9V_AV_1 (VCLZLSBB_V16QI, "vclzlsbb_v16qi", CONST, vclzlsbb_v16qi) >> -BU_P9V_AV_1 (VCLZLSBB_V8HI, "vclzlsbb_v8hi", CONST, vclzlsbb_v8hi) >> -BU_P9V_AV_1 (VCLZLSBB_V4SI, "vclzlsbb_v4si", CONST, vclzlsbb_v4si) >> -BU_P9V_AV_1 (VCTZLSBB_V16QI, "vctzlsbb_v16qi", CONST, vctzlsbb_v16qi) >> -BU_P9V_AV_1 (VCTZLSBB_V8HI, "vctzlsbb_v8hi", CONST, vctzlsbb_v8hi) >> -BU_P9V_AV_1 (VCTZLSBB_V4SI, "vctzlsbb_v4si", CONST, vctzlsbb_v4si) >> +BU_P9V_AV_1 (VCLZLSBB_V16QI, "vclzlsbb_v16qi", CONST, vctzlsbb_v16qi) >> +BU_P9V_AV_1 (VCLZLSBB_V8HI, "vclzlsbb_v8hi", CONST, vctzlsbb_v8hi) >> +BU_P9V_AV_1 (VCLZLSBB_V4SI, "vclzlsbb_v4si", CONST, vctzlsbb_v4si) >> +BU_P9V_AV_1 (VCTZLSBB_V16QI, "vctzlsbb_v16qi", CONST, vclzlsbb_v16qi) >> +BU_P9V_AV_1 (VCTZLSBB_V8HI, "vctzlsbb_v8hi", CONST, vclzlsbb_v8hi) >> +BU_P9V_AV_1 (VCTZLSBB_V4SI, "vctzlsbb_v4si", CONST, vclzlsbb_v4si) > Please change the default to be equal to the builtin name, so, the BE > version. We do that everywhere else as well, and it makes a lot more > sense (since everything in Power has BE numbering). > > The trunk version has this correct afaics?
No, trunk has this, for example: const signed int __builtin_altivec_vclzlsbb_v16qi (vsc); VCLZLSBB_V16QI vctzlsbb_v16qi {endian} So the backport matches what is on trunk. Throughout the new builtin infrastructure, the defaults are set for little-endian, and the "endian" flag changes behavior for big-endian. > >> --- a/gcc/testsuite/gcc.target/powerpc/vsu/vec-cntlz-lsbb-0.c >> +++ b/gcc/testsuite/gcc.target/powerpc/vsu/vec-cntlz-lsbb-0.c >> @@ -1,6 +1,7 @@ >> /* { dg-do compile { target { powerpc*-*-* } } } */ > (Delete the redundant target clause when modifying any testcase, please). Okay. > >> /* { dg-require-effective-target powerpc_p9vector_ok } */ >> /* { dg-options "-mdejagnu-cpu=power9" } */ >> +/* { dg-additional-options "-mbig" { target powerpc64le-*-* } } */ > You don't need the target clause, if it already is BE by default it does > not do anything to add it redundantly. > > But this is wrong anyway: the name of the target triple does not say > whether we are BE or LE. Instead you should use the be or le selectors. > But again, just add -mbig always. This was added by David Edelsohn to the trunk version of the patch, because -mbig actually is not supported on all subtargets. (I found that quite surprising also.) Apparently this doesn't work on AIX, for example. But -mlittle works everywhere. Go figure. That's something that should be fixed, I guess, but it's orthogonal to this patch. Thanks! Bill > >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/powerpc/vsu/vec-cntlz-lsbb-3.c >> @@ -0,0 +1,15 @@ >> +/* { dg-do compile { target { powerpc*-*-* } } } */ >> +/* { dg-require-effective-target powerpc_p9vector_ok } */ >> +/* { dg-options "-mdejagnu-cpu=power9 -mlittle" } */ > And here you do it correctly :-) > > Okay with those fixes (all happen a few times). Thanks! > > > Segher