Re: [PATCH 1/8] rs6000: More factoring of overload processing

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

On Fri, Jan 28, 2022 at 03:19:48PM -0600, Bill Schmidt wrote:
> On 1/28/22 1:11 PM, Segher Boessenkool wrote:
> > On Fri, Jan 28, 2022 at 11:50:19AM -0600, Bill Schmidt wrote:
> >> +     and the generic code will issue the appropriate error message.  Skip
> >> +     this test for functions where we don't fully describe all the possible
> >> +     overload signatures in rs6000-overload.def (because they aren't relevant
> >> +     to the expansion here).  If we don't, we get confusing error messages.  */
> >> +  if (fcode != RS6000_OVLD_VEC_PROMOTE
> >> +      && fcode != RS6000_OVLD_VEC_SPLATS
> >> +      && fcode != RS6000_OVLD_VEC_EXTRACT
> >> +      && fcode != RS6000_OVLD_VEC_INSERT
> >> +      && fcode != RS6000_OVLD_VEC_STEP
> >> +      && (!VOID_TYPE_P (TREE_VALUE (fnargs)) || n < nargs))
> >>      return NULL;
> > Can you expand a bit on this, give an example for example?  It is very
> > hard to understand this code, the way it depends on code following many
> > lines later.
> 
> Sure, sorry.
> 
> This check gives up if the number of arguments doesn't match the prototype.
> It gives a fairly generic error message.  That part of it has always been
> in here.
> 
> Now, I moved this check forward relative to the big switch statement on
> fcode, because there are redundant checks for the number of arguments
> in each of the resolve_vec_* helper functions.  This allowed me to simplify
> those a bit.
> 
> Now, it turns out that this doesn't work so well for functions that aren't
> fully described in rs6000-overload.def.  For example, for vec_splats we
> have:
> 
> ; There are no actual builtins for vec_splats.  There is special handling for
> ; this in altivec_resolve_overloaded_builtin in rs6000-c.cc, where the call
> ; is replaced by a constructor.  The single overload here causes
> ; __builtin_vec_splats to be registered with the front end so that can happen.
> [VEC_SPLATS, vec_splats, __builtin_vec_splats]
>   vsi __builtin_vec_splats (vsi);
>     ABS_V4SI SPLATS_FAKERY
> 
> So even though __builtin_vec_splats accepts all vector types, the
> infrastructure cheats and just records one prototype.  We end up getting
> an error message that refers to this specific prototype even when we are
> handling a different argument type.  That is completely confusing to the
> user.  So I felt I was starting to get too deep for a simple refactoring
> patch, and gave up on early number-of-arguments checking for the special
> cases that use the _FAKERY technique.
> 
> That's probably still not clear, but maybe clearer?

Much better, thanks!

So put a comment before the code handling the arg checking for
vec_splats etc. saying just that?  Or the much condensed form "these
codes should be handled separately because <bla>" :-)  (And the larger
explanation in the commit message -- there you can talk about the old
code / old situation as well :-) )

> >> +    default:
> >> +      ;
> > Don't.
> >
> > I like this better than a BS break statement, but it is just as stupid.
> >
> > If you need this, you don't want a switch statement, but some number of
> > if statements.  You cannot use a switch as a shorthand for this because
> > we have a silly warning and -Werror for this use.
> >
> > You probably get easier to understand code that way, too, you can get
> > rid of the above (just do some early returns), etc.
> 
> If I understand correctly, you'd like me to resubmit this in if-then-else
> form.  That's fine, just want to be sure that's what you want.

Yes please.  This is new code, so let's please keep it as readable as
possible.  Since you need to redo some of it anyway as well...


Segher