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

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

Hi!

On 11/17/21 5:06 PM, Bill Schmidt wrote:
> 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.

That said, I don't like the "requires" language at all, either.  How about
replacing "builtin X requires builtin Y" with "overloaded builtin X is
implemented by builtin Y" as a better explanation?

That could be done easily with a standalone patch that doesn't affect the
test suite, since none of the tests check for the note diagnostic.

Thanks,
Bill

>
> 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