On 11/10/21 2:33 AM, Segher Boessenkool wrote: > On Tue, Nov 09, 2021 at 03:46:54PM -0600, Bill Schmidt wrote: >> Hi! Some time ago I realized I hadn't tested the new builtin support >> against 32-bit >> big-endian in quite a while. When I did, I found a handful of errors that >> needed >> correcting. >> - One builtin needs to be disabled for 32-bit. >> - One builtin needs to be restricted to 32-bit only. >> - One builtin used unsigned long when it needed unsigned long long. >> - Six builtins used unsigned long long when they needed unsigned long. >> - One test case needed its expected error message adjusted. >> Otherwise things were fine. >> Bootstrapped and tested on powerpc64le-linux-gnu and powerpc64-linux-gnu >> with no >> regressions. > {-m32,-m64} for the latter, right?
Yes, indeed. > >> * config/rs6000/rs6000-builtin-new.def (CMPB): Flag as no32bit. >> (BPERMD): Flag as 32bit. >> (UNPACK_TD): Return unsigned long long instead of unsigned long. >> (SET_TEXASR): Pass unsigned long instead of unsigned long long. >> (SET_TEXASRU): Likewise. >> (SET_TFHAR): Likewise. >> (SET_TFIAR): Likewise. >> (TABORTDC): Likewise. >> (TABORTDCI): Likewise. >> * config/rs6000/rs6000-call.c (rs6000_expand_new_builtin): Fix error >> handling for no32bit. Add 32bit handling for RS6000_BIF_BPERMD. >> >> gcc/testsuite/ >> * gcc.target/powerpc/cmpb-3.c: Adjust error message. > >> const signed long __builtin_bpermd (signed long, signed long); >> - BPERMD bpermd_di {} >> + BPERMD bpermd_di {32bit} > That is not what the old code does? > > case POWER7_BUILTIN_BPERMD: > return rs6000_expand_binop_builtin (((TARGET_64BIT) > ? CODE_FOR_bpermd_di > : CODE_FOR_bpermd_si), exp, > target); Sorry! I mischaracterized what "32bit" implies. It means we have to perform special processing for 32-bit, not that it is restricted to 32-bit only. So below, you see the code added to handle it and subsitute the icode as required. >> - void __builtin_set_texasr (unsigned long long); >> + void __builtin_set_texasr (unsigned long); >> SET_TEXASR nothing {htm,htmspr} >> >> - void __builtin_set_texasru (unsigned long long); >> + void __builtin_set_texasru (unsigned long); >> SET_TEXASRU nothing {htm,htmspr} >> >> - void __builtin_set_tfhar (unsigned long long); >> + void __builtin_set_tfhar (unsigned long); >> SET_TFHAR nothing {htm,htmspr} >> >> - void __builtin_set_tfiar (unsigned long long); >> + void __builtin_set_tfiar (unsigned long); >> SET_TFIAR nothing {htm,htmspr} > This does not seem to be what the exiting code does, either? Try with > -m32 -mpowerpc64 (it extends to 64 bit there, so the builtin does not > have long int as parameter, it has long long int). This uses a tfiar_t, which is a typedef for uintptr_t, so long int is appropriate. This is necessary to make the HTM tests pass on 32-bit powerpc64. > >> @@ -15758,6 +15759,8 @@ rs6000_expand_new_builtin (tree exp, rtx target, >> { >> if (fcode == RS6000_BIF_MFTB) >> icode = CODE_FOR_rs6000_mftb_si; >> + else if (fcode == RS6000_BIF_BPERMD) >> + icode = CODE_FOR_bpermd_si; >> else >> gcc_unreachable (); >> } > But you disabled it for 32 bit now, so huh. See above for my former mis-explanation. Oops... > >> --- a/gcc/testsuite/gcc.target/powerpc/cmpb-3.c >> +++ b/gcc/testsuite/gcc.target/powerpc/cmpb-3.c >> @@ -8,7 +8,7 @@ void abort (); >> long long int >> do_compare (long long int a, long long int b) >> { >> - return __builtin_cmpb (a, b); /* { dg-error "'__builtin_cmpb' is not >> supported in this compiler configuration" } */ >> + return __builtin_cmpb (a, b); /* { dg-error "'__builtin_p6_cmpb' is >> not supported in 32-bit mode" } */ >> } > The original spelling is the correct one? This is something I have on my to-do list for the future, to see whether I can improve it. The overloaded function __builtin_cmpb gets translated to the underlying non-overloaded builtin __builtin_p6_cmpb, and that's the only name that's still around by the time we get to the error processing. I want to see whether I can add some infrastructure to recover the overloaded function name in such cases. Is it okay to defer this for now? Thanks for the review! Bill > > > Segher