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
