Hi Segher,
On 8/27/21 6:07 PM, Segher Boessenkool wrote:
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).
OK. This requires a small change to the gen program to match.
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).
Yes, it's a fair point to figure out whether this should be refactored
better. This particular function is used both by the overloading
support and in other places, so it really belongs elsewhere, I think.
In general, we might consider simplifying the target hooks that land in
the C frontend file and move the guts into a separate builtin support
file. Certainly rs6000-call.c is still too large, and factoring out the
builtins parts would be an improvement. I'll put this on the list for
follow-up after the main series lands.
+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?
Yeah, how'd that happen...
+ 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?
I don't understand how TYPE_MAIN_VARIANT is appropriate. Please explain?
+ /* 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)
Yeah, there is a lot of bad formatting in the copied code. I probably
should have tried to clean it all up, but generally just did where I was
making specific changes. I'll see what I can do about these things.
+ 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".
It does indeed. Let me put that on the list for after the main patch
series is done, if that's ok.
+ 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!
Good call.
+ break;
+ };
Stray semicolon. Did this not warn?
It did not! Or at least I didn't notice if it did. I believe that
-Werror may be somehow turned off for this file due to a bunch of
excessive warnings involving format errors, though. I've been meaning
to look into how that's happening...
Could you please try to factor this better?
I promise to do so later. Right now I'm trying hard not to screw up the
logic of this messed-up beast. I think having it committed in the
messier, but closer to original, state will be a good intermediate
step. I definitely want to take a shot at making this better down the road.
Thanks very much for the review!
Bill
Segher
