On 1/28/22 1:11 PM, Segher Boessenkool wrote:
> On Fri, Jan 28, 2022 at 11:50:19AM -0600, Bill Schmidt wrote:
>> This patch continues the refactoring started with r12-6014.
> ab3f5b71dc6e
>
>> + 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?
>
>> + 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.
Thanks for the review!
Bill
>
>
> Segher
