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

Reply via email to