Paths
| Differential D106409
[PowerPC] Truncate exponent parameter for vec_cts,vec_ctf ClosedPublic Authored by ZarkoCA on Jul 20 2021, 4:07 PM.
Details
Summary On PowerPC, the vec_ct* builtin function take the form of eg. d=vec_cts(a,b) Further documentation for the builtins can be found here:
Diff Detail
Event TimelineComment Actions Thanks for this patch @ZarkoCA. We have six of these functions in altivec: vec_ctd, vec_ctf, vec_cts, vec_ctsl, vec_ctu, vec_ctul. They are all defined as macros in altivec.h and not all of them map to the builtins checked in this patch (eg vec_ctul). I realize this is an improvement over what we have, but just wondering if you've considered diagnosing cases like vec_ctul or the following: vector double a; vector unsigned int res = vec_ctu(a, 32); One solution I can think of is to introduce a noop pass-through builtin whose only purpose would be to allow semantic checking in places like Sema::CheckPPCBuiltinFunctionCall. For example say the builtin is called _builtin_pass_through_with_rcheck_1_32 then we can change vec_ctul in altivec.h to the following: #define vec_ctul(__a, __b) \
_Generic((__a), vector float \
: __extension__({ \
vector float __ret = \
(vector float)(__a) * \
(vector float)(vector unsigned)((0x7f + (__builtin_pass_through_with_rcheck_1_32(__b))) << 23); \
__builtin_vsx_xvcvspuxds( \
__builtin_vsx_xxsldwi(__ret, __ret, 1)); \
}), \and the sema check would look something like: case PPC::BI__builtin_pass_through_with_rcheck_1_32:
return SemaBuiltinConstantArgRange(TheCall, 0, 0, 31);Any thoughts @nemanjai ? Comment Actions My preference would be to just truncate the value with & 0x1F. It won't produce any code in the binary and will work as expected. Comment Actions
That's fine with me too. ZarkoCA retitled this revision from [PowerPC] Add diagnostic for out of range values for vec_cts,vec_ctf to [PowerPC] Truncate results for out of range values for vec_cts,vec_ctf. Comment ActionsSwitch direction of patch as per reviewer comments. We now truncate the results and there is no diagnostic. Comment Actions I may be wrong, but I really think this is incorrect. Please do some functional (execution) testing on this. Also, please re-title this from "truncate results" to something like "truncate exponent parameter" or similar since you are not truncating the result but the parameter.
This revision now requires changes to proceed.Sep 30 2021, 3:06 AM ZarkoCA retitled this revision from [PowerPC] Truncate results for out of range values for vec_cts,vec_ctf to [PowerPC] Truncate exponent parameter for vec_cts,vec_ctf.Oct 1 2021, 2:09 PM Comment Actions I believe you are planning an update for this patch. Requesting changes to take it off the queue until you have uploaded the updated version. This revision now requires changes to proceed.Nov 5 2021, 3:35 AM This revision is now accepted and ready to land.Jun 16 2023, 8:24 AM This revision was landed with ongoing or failed builds.Jul 11 2023, 8:52 AM Closed by commit rG14742f2a689c: [PowerPC] Truncate exponent parameter for vec_cts,vec_ctf (authored by ZarkoCA). · Explain Why This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 539137 clang/lib/Headers/altivec.h
clang/test/CodeGen/ppc-vec_ct-truncate.c
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Hmm... aren't you truncating the wrong thing here (and for all the other shifted ones? Shouldn't it be something like: