Re: [PATCH] rs6000: Builtins test changes for BFP scalar tests

[Bill Schmidt <wschmidt@linux.ibm.com>]

On 11/17/21 3:32 PM, Segher Boessenkool wrote:
> On Wed, Nov 17, 2021 at 02:58:54PM -0600, Bill Schmidt wrote:
>> Hi!  This patch is broken out of the previous patch for all the builtins test
>> suite adjustments.  Here we have some slight changes in error messages due to
>> how the internals have changed between the old and new builtins methods.
>>
>> For scalar-extract-exp-2.c we change:
>>   error: '__builtin_vec_scalar_extract_exp is not supported in this compiler configuration'
>>
>> to:
>>   error: '__builtin_vsx_scalar_extract_exp' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option
>>   note: builtin '__builtin_vec_scalar_extract_exp' requires builtin '__builtin_vsx_scalar_extract_exp'
> I don't like that at all.  The user didn't write the _vsx thing, and it
> isn't documented either (neither is the _vec one, but that is a separate
> issue, specific to this builtin).

I feel like I haven't explained this well.  This kind of thing has been in
existence forever even in the old builtins code.  The combination of the
error showing the internal builtin name, and the note tying the overload
name to the internal builtin name, has been there all along.  The name of
the internal builtin is pretty meaningless.  The only thing that's interesting
in this case is that we previously didn't get this *for this specific case*
because the old code went to a generic fallback.  But in many other cases
you get exactly this same kind of error message for the old code.

>
>> The new message provides more information.  In both cases, it is less than
>> ideal that we don't refer to scalar_extract_exp, which is referenced in
>> the source line, but this is because scalar_extract_exp is #define'd to
>> __builtin_vec_scalar_extract_exp, so it's unavoidable.  Certainly this is no
>> worse than before, and arguably better.
> It is a macro, enough said there
>
> The __builtin_ implementation should be documented (in the GCC manual,
> if not elsewhere).  The warnings should talk about _vec, because the
> _vsx thing only exists as implementation detail, and we should never
> talk about those.  We don't have errors about adddi3 either!
>
>>   error: '__builtin_vsx_scalar_extract_sig' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option
>>   note: builtin '__builtin_vec_scalar_extract_sig' requires builtin '__builtin_vsx_scalar_extract_sig'
> The rhs in the note does not *exist*, as far as the user is concerned.
> One builtin requiring another is all gobbledygook.

As stated above, this isn't something new that I've added.  That's what
we already do.  It's how the overload error messages have always been.

I haven't been able to eradicate everything awful here...

Thanks,
Bill

>
>> For scalar-test-neg-{2,3,5}.c, we actually change the test case.  This is
>> because we deliberately removed some undocumented and pointless	overloads,
>> where each overload mapped to a single builtin.  These were:
>> 	__builtin_vec_scalar_test_neg_sp
>> 	__builtin_vec_scalar_test_neg_dp
>> 	__builtin_vec_scalar_test_neg_qp
>> which are redundant with the "real" overload:
>> 	__builtin_vec_scalar_test_neg
>> The latter maps to three builtins of the appropriate type.
> Yes.  And the new ones are undocumented and useless just as well, they
> just have better names.
>
>
> Segher