[PATCH] rs6000: Builtins test changes for BFP scalar tests
Thu Nov 18 21:59:41 GMT 2021
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.
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
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?
More information about the Gcc-patches