Hi Segher,
On 8/25/21 6:27 PM, Segher Boessenkool wrote:
On Thu, Jul 29, 2021 at 08:31:01AM -0500, Bill Schmidt wrote:
* config/rs6000/rs6000-overload.def: Add remaining overloads.
+; TODO: Note that the entry for VEC_ADDE currently gets ignored in
+; altivec_resolve_overloaded_builtin. Revisit whether we can remove
+; that. We still need to register the legal builtin forms here.
+[VEC_ADDE, vec_adde, __builtin_vec_adde]
+ vsq __builtin_vec_adde (vsq, vsq, vsq);
+ VADDEUQM VADDEUQM_VSQ
+ vuq __builtin_vec_adde (vuq, vuq, vuq);
+ VADDEUQM VADDEUQM_VUQ
I'm not sure what this means. "Currently" is the problem I think. Do
you mean that the existing code (before this patch) ignores it already?
Right, exactly. There is special-case code there for handling ADDE
builtins, as there is for a number of other cases. As a future cleanup,
we'd like to have as little special-case code as possible. For this
conversion effort, I elected to wait with that and leave the TODO here
in the file.
+; #### XVRSPIP{TARGET_VSX};VRFIP
+[VEC_CEIL, vec_ceil, __builtin_vec_ceil]
+ vf __builtin_vec_ceil (vf);
+ VRFIP
+ vd __builtin_vec_ceil (vd);
+ XVRDPIP
Is that a comment you forgot to remove, or is there work to be done here?
Sorry about these. For a number of overloads, there is one builtin we
have to use in case VSX isn't available, but a better one we can use if
it is. I had been thinking about adding special syntax in this file to
select between multiple builtins. However, since then I've realized
it's probably better to have a single built-in mapping to a pattern that
dispatches to the other two based on TARGET_VSX being available. We
probably have support for all these cases like that in vector.md as it is.
tl;dr: I'll remove these comments. :-)
+[VEC_CMPEQ, vec_cmpeq, __builtin_vec_cmpeq]
+; #### XVCMPEQSP{TARGET_VSX};VCMPEQFP
And this.
+; #### XVCMPEQSP_P{TARGET_VSX};VCMPEQFP_P
+[VEC_CMPEQ_P, SKIP, __builtin_vec_vcmpeq_p]
And more!
And more later. It isn't clear to me at all what those comments mean,
and they are formatted haphazardly, so looks like a WIP?
+; Note that the entries for VEC_MUL are currently ignored. See rs6000-c.c:
+; altivec_resolve_overloaded_builtin, where there is special-case code for
+; VEC_MUL. TODO: Is this really necessary? Investigate. Seven missing
+; prototypes here...no corresponding builtins. Also added "vmulld" in P10
+; which could be used instead of MUL_V2DI, conditionally?
Space after "..." :-P
+; Opportunity for improvement: We can use XVRESP instead of VREFP for
+; TARGET_VSX. We would need conditional dispatch to allow two possibilities.
+; Some syntax like "XVRESP{TARGET_VSX};VREFP".
+; TODO. ####
+[VEC_RE, vec_re, __builtin_vec_re]
Don't we already anyway? The only difference is whether all VSRs are
allowed or only the VRs, no? The RTL generated is just the same? Or
maybe I am overlooking something :-)
Right, this is the same conclusion I came to -- I should be able to just
use vector.md pattern names. Future improvement (and maybe altogether
unnecessary; I'll do some testing).
+; **************************************************************************
+; **************************************************************************
+; **** Deprecated overloads that should never have existed at all ****
+; **************************************************************************
+; **************************************************************************
The coding conventions say not to use showy block comments like that,
but it seems appropriate here :-)
Okay for trunk with the #### looked at. Please don't repost this one.
Thanks!
Awww. :-P
Thanks very much for the review! I know these were tedious to look
through, and I appreciate it very much.
Bill
Segher
