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

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

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