Hi!
On Thu, Jul 29, 2021 at 08:31:06AM -0500, Bill Schmidt wrote:
> Although this patch looks quite large, the changes are fairly minimal.
> Most of it is duplicating the large function that does the overload
> resolution using the automatically generated data structures instead of
> the old hand-generated ones. This doesn't make the patch terribly easy to
> review, unfortunately.
Yeah, and it pretty important that it does the same as the old code did.
> Just be aware that generally we aren't changing
> the logic and functionality of overload handling.
Good :-)
> * config/rs6000/rs6000-c.c (rs6000-builtins.h): New include.
> (altivec_resolve_new_overloaded_builtin): New forward decl.
> (rs6000_new_builtin_type_compatible): New function.
> (altivec_resolve_overloaded_builtin): Call
> altivec_resolve_new_overloaded_builtin.
> (altivec_build_new_resolved_builtin): New function.
> (altivec_resolve_new_overloaded_builtin): Likewise.
> * config/rs6000/rs6000-call.c (rs6000_new_builtin_is_supported_p):
> Likewise.
No "_p" please (it already has a verb in the name, and explicit ones are
much clearer anyway).
Does everything else belong in the C frontend file but this last
function not? Maybe this could be split up better. Maybe there should
be a separate file for just the builtin support, it probably is big
enough?
(This is all for later of course, but please think about it. Code
rearrangement (or even refactoring) can be done at any time, there is
no time pressure on it).
> +static tree
> +altivec_resolve_new_overloaded_builtin (location_t, tree, void *);
This fits on one line, please do so (this is a declaration, not a
function definition; those are put with the name in the first column,
to make searching for them a lot easier).
> +static bool
> +rs6000_new_builtin_type_compatible (tree t, tree u)
> +{
> + if (t == error_mark_node)
> + return false;
> +
> + if (INTEGRAL_TYPE_P (t) && INTEGRAL_TYPE_P (u))
> + return true;
> +
> + if (TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128
> + && is_float128_p (t) && is_float128_p (u))
Indent is wrong here?
> + if (POINTER_TYPE_P (t) && POINTER_TYPE_P (u))
> + {
> + t = TREE_TYPE (t);
> + u = TREE_TYPE (u);
> + if (TYPE_READONLY (u))
> + t = build_qualified_type (t, TYPE_QUAL_CONST);
> + }
Just use TYPE_MAIN_VARIANT, don't make garbage here?
> + /* The AltiVec overloading implementation is overall gross, but this
> + is particularly disgusting. The vec_{all,any}_{ge,le} builtins
> + are completely different for floating-point vs. integer vector
> + types, because the former has vcmpgefp, but the latter should use
> + vcmpgtXX.
Yes. The integer comparisons were reduced to just two in original VMX
(eq and gt, well signed and unsigned versions of the latter), but that
cannot be done for floating point (all of a<b a==b a>b can be false at
the same time (for a or b NaN), so this cannot be compressed to just two
functions, we really need three at least).
We have the same thing in xscmp* btw, it's not just VMX. Having three
ops for the majority of comparisons (and doing the rest with two or so)
is nicer than having 14 :-)
There aren't builtins for most of that, thankfully.
> + if (TARGET_DEBUG_BUILTIN)
> + fprintf (stderr, "altivec_resolve_overloaded_builtin, code = %4d, %s\n",
> + (int)fcode, IDENTIFIER_POINTER (DECL_NAME (fndecl)));
(space after cast, also in debug code)
> + const char *name
> + = fcode == RS6000_OVLD_VEC_ADDE ? "vec_adde": "vec_sube";
(space before and after colon)
> + const char *name = fcode == RS6000_OVLD_VEC_ADDEC ?
> + "vec_addec": "vec_subec";
(ditto. also, ? cannot end a line. maybe just
const char *name;
name = fcode == RS6000_OVLD_VEC_ADDEC ? "vec_addec" : "vec_subec";
)
> + const char *name
> + = fcode == RS6000_OVLD_VEC_SPLATS ? "vec_splats": "vec_promote";
(more)
> + case E_SFmode: type = V4SF_type_node; size = 4; break;
> + case E_DFmode: type = V2DF_type_node; size = 2; break;
Don't put multiple statements on one line. Put the label on its own,
too, for that matter.
> + }
> + return build_constructor (type, vec);
This is wrong indenting. Where it started, I have no idea. You figure
it out :-)
> + bad:
> + {
> + const char *name = rs6000_overload_info[adj_fcode].ovld_name;
> + error ("invalid parameter combination for AltiVec intrinsic %qs", name);
> + return error_mark_node;
> + }
A huge function with a lot of "goto bad;" just *screams* "this needs to
be factored".
> + case ENB_P5:
> + if (!TARGET_POPCNTB)
> + return false;
> + break;
case ENB_P5:
return TARGET_POPCNTB;
and similar for all further cases. It is shorter and does not have
negations, win-win!
> + break;
> + };
Stray semicolon. Did this not warn?
Could you please try to factor this better?
Segher
