rs6000: Builtins test changes for BFP scalar tests

  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'

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.

The cases for:
	scalar-insert-exp-2.c
	scalar-insert-exp-5.c
	scalar-insert-exp-8.c
are all similar.

For scalar-extract-sig-2.c we again change:
  error: '__builtin_vec_scalar_extract_sig' is not supported in this compiler configuration'

to:
  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'

Here it is clearer because there is no #define to muddy things up, and
again the new message is arguably better than the old.

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.

The revised test case uses the "real" overload instead, and otherwise the
changes to the error messages are the same as for all the other cases.

2021-11-17  Bill Schmidt  <wschmidt@linux.ibm.com>

gcc/testsuite/
	* gcc.target/powerpc/bfp/scalar-extract-exp-2.c: Adjust error
	message.
	* gcc.target/powerpc/bfp/scalar-extract-sig-2.c: Likewise.
	* gcc.target/powerpc/bfp/scalar-insert-exp-2.c: Likewise.
	* gcc.target/powerpc/bfp/scalar-insert-exp-5.c: Likewise.
	* gcc.target/powerpc/bfp/scalar-insert-exp-8.c: Likewise.
	* gcc.target/powerpc/bfp/scalar-test-neg-2.c: Likewise.
	* gcc.target/powerpc/bfp/scalar-test-neg-3.c: Likewise.
	* gcc.target/powerpc/bfp/scalar-test-neg-5.c: Likewise.
---
 gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c | 2 +-
 gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c | 2 +-
 gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c  | 2 +-
 gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c  | 2 +-
 gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c  | 2 +-
 gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c    | 2 +-
 gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c    | 2 +-
 gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c    | 2 +-
 8 files changed, 8 insertions(+), 8 deletions(-)
  

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
  

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
  

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
  

Hi!

On Wed, Nov 17, 2021 at 05:06:05PM -0600, Bill Schmidt wrote:
> > 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.

Yes.  And it still is a regression (in *this* case).


Segher
  

On Thu, Nov 18, 2021 at 07:32:27AM -0600, Bill Schmidt wrote:
> >>>   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 is better (although builtin Y *does not exist* as far as the user
is concerned: it is not documented, and you cannot write it in source
code afaics).


Segher
  

On 11/18/21 3:16 PM, Segher Boessenkool wrote:
> Hi!
>
> On Wed, Nov 17, 2021 at 05:06:05PM -0600, Bill Schmidt wrote:
>>> 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.
> Yes.  And it still is a regression (in *this* case).

Sorry, I don't understand.  Why specifically is this a regression?

Bill

>
>
> Segher
  

On Thu, Nov 18, 2021 at 03:30:48PM -0600, Bill Schmidt wrote:
> 
> On 11/18/21 3:16 PM, Segher Boessenkool wrote:
> > Hi!
> >
> > On Wed, Nov 17, 2021 at 05:06:05PM -0600, Bill Schmidt wrote:
> >>> 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.
> > Yes.  And it still is a regression (in *this* case).
> 
> Sorry, I don't understand.  Why specifically is this a regression?

It is wrong now, in ways that it wasn't wrong before.  That is the
definition of regression!


Segher
  

Hi!

On 11/18/21 3:32 PM, Segher Boessenkool wrote:
> On Thu, Nov 18, 2021 at 03:30:48PM -0600, Bill Schmidt wrote:
>> On 11/18/21 3:16 PM, Segher Boessenkool wrote:
>>> Hi!
>>>
>>> On Wed, Nov 17, 2021 at 05:06:05PM -0600, Bill Schmidt wrote:
>>>>> 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.
>>> Yes.  And it still is a regression (in *this* case).
>> Sorry, I don't understand.  Why specifically is this a regression?
> It is wrong now, in ways that it wasn't wrong before.  That is the
> definition of regression!

I'm sorry, I disagree.  With clarification of the note, I don't see how
this can be considered a regression.  It is providing information in a
different way, but it is still clear that the user has misused the builtin
in the context, and, unlike before, it now tells them *what* is wrong with
the options that were used (not just "unavailable in this configuration").
The fact that an internal builtin name is *also* disclosed as part of
this does not make it wrong.

The way that overloads work, we can only tell whether a builtin is
enabled with the current set of options by looking at the true builtin
that the overload maps to.  The enablement checking code doesn't have
any knowledge that an overloaded function maps to it.  So that error
message is produced without knowledge of the overloading.  The note
diagnostic is added by the overload processing code that *is* aware
that the mapping exists.

The enablement checking code (rs6000_invalid_builtin in the old code,
rs6000_invalid_new_builtin in the new code) is called from multiple
places, and not always in an overload context, so we can't assume
this is the case.  Not all builtins are mapped to by overloads, but
they still need enablement checking.

Would it be possible to change things so that we pass in the overload
name to be used in the error message when appropriate?  Yes.  But
this would have a much larger impact on the test suite than the
current method, because all error tests for overloads would now
have to change.  That is, there are many existing tests that are
already "wrong" in the sense that they report the internal builtin
name.

I suggest that we add that to the list of future cleanups due to
the size of the impact.  I agree that it has never been ideal to
mention the internal builtin name on these messages.  It's just
not unique to the changes I've made here; it's a pre-existing
condition that needs work to cleanse it everywhere.

Can we move forward this way?

Thanks,
Bill

>
>
> Segher
  

Hi!  I'd like to ping this patch.  Segher had objected to the change in diagnostics,
but I hope we've solved that now with the better informational message [1].

Thanks!
Bill

[1] https://gcc.gnu.org/pipermail/gcc-patches/2021-November/585250.html

On 11/17/21 2:58 PM, 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'
>
> 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.
>
> The cases for:
> 	scalar-insert-exp-2.c
> 	scalar-insert-exp-5.c
> 	scalar-insert-exp-8.c
> are all similar.
>
> For scalar-extract-sig-2.c we again change:
>   error: '__builtin_vec_scalar_extract_sig' is not supported in this compiler configuration'
>
> to:
>   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'
>
> Here it is clearer because there is no #define to muddy things up, and
> again the new message is arguably better than the old.
>
> 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.
>
> The revised test case uses the "real" overload instead, and otherwise the
> changes to the error messages are the same as for all the other cases.
>
> 2021-11-17  Bill Schmidt  <wschmidt@linux.ibm.com>
>
> gcc/testsuite/
> 	* gcc.target/powerpc/bfp/scalar-extract-exp-2.c: Adjust error
> 	message.
> 	* gcc.target/powerpc/bfp/scalar-extract-sig-2.c: Likewise.
> 	* gcc.target/powerpc/bfp/scalar-insert-exp-2.c: Likewise.
> 	* gcc.target/powerpc/bfp/scalar-insert-exp-5.c: Likewise.
> 	* gcc.target/powerpc/bfp/scalar-insert-exp-8.c: Likewise.
> 	* gcc.target/powerpc/bfp/scalar-test-neg-2.c: Likewise.
> 	* gcc.target/powerpc/bfp/scalar-test-neg-3.c: Likewise.
> 	* gcc.target/powerpc/bfp/scalar-test-neg-5.c: Likewise.
> ---
>  gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c | 2 +-
>  gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c | 2 +-
>  gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c  | 2 +-
>  gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c  | 2 +-
>  gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c  | 2 +-
>  gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c    | 2 +-
>  gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c    | 2 +-
>  gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c    | 2 +-
>  8 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c
> index 922180675fc..53b67c95cf9 100644
> --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c
> +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c
> @@ -14,7 +14,7 @@ get_exponent (double *p)
>  {
>    double source = *p;
>  
> -  return scalar_extract_exp (source);	/* { dg-error "'__builtin_vec_scalar_extract_exp' is not supported in this compiler configuration" } */
> +  return scalar_extract_exp (source);	/* { dg-error "'__builtin_vsx_scalar_extract_exp' requires the" } */
>  }
>  
>  
> diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c
> index e24d4bd23fe..39ee74c94dc 100644
> --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c
> +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c
> @@ -12,5 +12,5 @@ get_significand (double *p)
>  {
>    double source = *p;
>  
> -  return __builtin_vec_scalar_extract_sig (source); /* { dg-error "'__builtin_vec_scalar_extract_sig' is not supported in this compiler configuration" } */
> +  return __builtin_vec_scalar_extract_sig (source); /* { dg-error "'__builtin_vsx_scalar_extract_sig' requires the" } */
>  }
> diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c
> index feb943104da..efd69725905 100644
> --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c
> +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c
> @@ -16,5 +16,5 @@ insert_exponent (unsigned long long int *significand_p,
>    unsigned long long int significand = *significand_p;
>    unsigned long long int exponent = *exponent_p;
>  
> -  return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vec_scalar_insert_exp' is not supported in this compiler configuration" } */
> +  return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vsx_scalar_insert_exp' requires the" } */
>  }
> diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c
> index 0e5683d5d1a..f85966a6fdf 100644
> --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c
> +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c
> @@ -16,5 +16,5 @@ insert_exponent (double *significand_p,
>    double significand = *significand_p;
>    unsigned long long int exponent = *exponent_p;
>  
> -  return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vec_scalar_insert_exp' is not supported in this compiler configuration" } */
> +  return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vsx_scalar_insert_exp_dp' requires the" } */
>  }
> diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c
> index bd68f770985..b1be8284b4e 100644
> --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c
> +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c
> @@ -16,5 +16,5 @@ insert_exponent (unsigned __int128 *significand_p, /* { dg-error "'__int128' is
>    unsigned __int128 significand = *significand_p;  /* { dg-error "'__int128' is not supported on this target" } */
>    unsigned long long int exponent = *exponent_p;
>  
> -  return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vec_scalar_insert_exp' is not supported in this compiler configuration" } */
> +  return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vsx_scalar_insert_exp' requires the" } */
>  }
> diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c
> index 7d2b4deefc3..46d743a899b 100644
> --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c
> +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c
> @@ -10,5 +10,5 @@ test_neg (float *p)
>  {
>    float source = *p;
>  
> -  return __builtin_vec_scalar_test_neg_sp (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_sp' requires" } */
> +  return __builtin_vec_scalar_test_neg (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_sp' requires" } */
>  }
> diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c
> index b503dfa8b56..bfc892b116e 100644
> --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c
> +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c
> @@ -10,5 +10,5 @@ test_neg (double *p)
>  {
>    double source = *p;
>  
> -  return __builtin_vec_scalar_test_neg_dp (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_dp' requires" } */
> +  return __builtin_vec_scalar_test_neg (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_dp' requires" } */
>  }
> diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c
> index bab86040a7b..8c55c1cfb5c 100644
> --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c
> +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c
> @@ -10,5 +10,5 @@ test_neg (__ieee128 *p)
>  {
>    __ieee128 source = *p;
>  
> -  return __builtin_vec_scalar_test_neg_qp (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_qp' requires" } */
> +  return __builtin_vec_scalar_test_neg (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_qp' requires" } */
>  }
  

On Thu, Nov 18, 2021 at 03:59:41PM -0600, Bill Schmidt wrote:
> On 11/18/21 3:32 PM, Segher Boessenkool wrote:
> > On Thu, Nov 18, 2021 at 03:30:48PM -0600, Bill Schmidt wrote:
> >> On 11/18/21 3:16 PM, Segher Boessenkool wrote:
> >>> On Wed, Nov 17, 2021 at 05:06:05PM -0600, Bill Schmidt wrote:
> >>>>> 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.
> >>> Yes.  And it still is a regression (in *this* case).
> >> Sorry, I don't understand.  Why specifically is this a regression?
> > It is wrong now, in ways that it wasn't wrong before.  That is the
> > definition of regression!
> 
> I'm sorry, I disagree.  With clarification of the note, I don't see how
> this can be considered a regression.  It is providing information in a
> different way, but it is still clear that the user has misused the builtin
> in the context, and, unlike before, it now tells them *what* is wrong with
> the options that were used (not just "unavailable in this configuration").
> The fact that an internal builtin name is *also* disclosed as part of
> this does not make it wrong.

It is claiming something is wrong with something the user didn't write.
The blow is softened a lot by the inform(), but it still is a
regression.  But you said you'll look into it, and it will be okay for
now, it isn't like this is super important.  Just an ugly wart :-)

> The way that overloads work, we can only tell whether a builtin is
> enabled with the current set of options by looking at the true builtin
> that the overload maps to.  The enablement checking code doesn't have
> any knowledge that an overloaded function maps to it.  So that error
> message is produced without knowledge of the overloading.  The note
> diagnostic is added by the overload processing code that *is* aware
> that the mapping exists.

Yeah, but you can keep track of what the original name was, somewhere.
Hopefully, anyway :-)

> The enablement checking code (rs6000_invalid_builtin in the old code,
> rs6000_invalid_new_builtin in the new code) is called from multiple
> places, and not always in an overload context, so we can't assume
> this is the case.  Not all builtins are mapped to by overloads, but
> they still need enablement checking.

A direct call would not get the "this comes from <this> overload" field
set, so that is easy to see in the error message code.

> Would it be possible to change things so that we pass in the overload
> name to be used in the error message when appropriate?  Yes.  But
> this would have a much larger impact on the test suite than the
> current method, because all error tests for overloads would now
> have to change.  That is, there are many existing tests that are
> already "wrong" in the sense that they report the internal builtin
> name.
> 
> I suggest that we add that to the list of future cleanups due to
> the size of the impact.  I agree that it has never been ideal to
> mention the internal builtin name on these messages.  It's just
> not unique to the changes I've made here; it's a pre-existing
> condition that needs work to cleanse it everywhere.
> 
> Can we move forward this way?

We can yes :-)  And thanks in advance!  It would be nice if we could
see this in GCC 12 still.  It is fine for stage 4 still, if it is error
message only (and a lot of testsuite :-) )


Segher
  

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'

So, okay for trunk.  Thanks!

One thing though:
> gcc/testsuite/
> 	* gcc.target/powerpc/bfp/scalar-extract-exp-2.c: Adjust error
> 	message.

Don't wrap unnecessarily please.

> 	* gcc.target/powerpc/bfp/scalar-extract-sig-2.c: Likewise.
> 	* gcc.target/powerpc/bfp/scalar-insert-exp-2.c: Likewise.
> 	* gcc.target/powerpc/bfp/scalar-insert-exp-5.c: Likewise.
> 	* gcc.target/powerpc/bfp/scalar-insert-exp-8.c: Likewise.
> 	* gcc.target/powerpc/bfp/scalar-test-neg-2.c: Likewise.
> 	* gcc.target/powerpc/bfp/scalar-test-neg-3.c: Likewise.
> 	* gcc.target/powerpc/bfp/scalar-test-neg-5.c: Likewise.


Segher