Page MenuHomePhabricator

[PowerPC] Truncate results for out of range values for vec_cts,vec_ctf
Needs ReviewPublic

Authored by ZarkoCA on Jul 20 2021, 4:07 PM.

Details

Reviewers
bmahjour
nemanjai
jsji
Group Reviewers
Restricted Project
Summary

LLVM (llc) will crash when a user specifies a number out of the allowed range (0-31) for b.
This patch truncates the results so that we avoid the backend crash in some cases.

Further documentation for the builtins can be found here:
https://www.ibm.com/docs/en/xl-c-and-cpp-linux/16.1.0?topic=functions-vec-ctf
https://www.ibm.com/docs/en/xl-c-and-cpp-linux/16.1.0?topic=functions-vec-cts

Diff Detail

Event Timeline

ZarkoCA created this revision.Jul 20 2021, 4:07 PM
ZarkoCA requested review of this revision.Jul 20 2021, 4:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2021, 4:07 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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 ?

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.

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.

That's fine with me too.

ZarkoCA planned changes to this revision.Jul 22 2021, 3:55 AM
ZarkoCA updated this revision to Diff 367525.Aug 19 2021, 9:27 AM
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.
ZarkoCA edited the summary of this revision. (Show Details)

Switch direction of patch as per reviewer comments. We now truncate the results and there is no diagnostic.
Edited title and summary to reflect this.