This is an archive of the discontinued LLVM Phabricator instance.

[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)
LLVM (llc) will crash when a user specifies a number out of the allowed range
(0-31) for b.This patch truncates b 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.

nemanjai requested changes to this revision.Sep 30 2021, 3:06 AM

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.

clang/lib/Headers/altivec.h
3178

Hmm... aren't you truncating the wrong thing here (and for all the other shifted ones? Shouldn't it be something like:

(vector float)(vector unsigned)((0x7f - ((__b) & 0x1F)) << 23)),
clang/test/CodeGen/builtins-ppc-xlcompat.c
25

Notice that all the fmul's multiply by zero. This really doesn't seem right - and is likely due to shifting left and then truncating (i.e. you shift a value left by 23/52 bits, then you bitwise-and it with 0x1F - all you're going to get are zeros).

This revision now requires changes to proceed.Sep 30 2021, 3:06 AM
ZarkoCA updated this revision to Diff 376630.EditedOct 1 2021, 2:07 PM
ZarkoCA edited the summary of this revision. (Show Details)
  • Correctly truncate b
  • Add a proper test case
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
ZarkoCA marked an inline comment as done.Oct 1 2021, 2:15 PM
ZarkoCA added inline comments.
clang/lib/Headers/altivec.h
3178

Thanks, I wasn't doing the truncation correctly. Redid to what I think is correct now.

nemanjai requested changes to this revision.Nov 5 2021, 3:35 AM

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
jsji resigned from this revision.Jun 2 2022, 7:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 7:59 AM
ZarkoCA updated this revision to Diff 528492.Jun 5 2023, 10:08 AM
ZarkoCA marked an inline comment as done.
ZarkoCA removed a reviewer: jsji.

Rebase and fix test.

nemanjai accepted this revision.Jun 16 2023, 8:24 AM

LGTM.

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
This revision was automatically updated to reflect the committed changes.