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

[Segher Boessenkool <segher@kernel.crashing.org>]

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

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

> 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