-
Notifications
You must be signed in to change notification settings - Fork 15k
[clang] Implement __builtin_stdc_rotate_left, __builtin_stdc_rotate_right #160259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14720,23 +14720,17 @@ bool IntExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E, | |
| case Builtin::BI__builtin_rotateleft16: | ||
| case Builtin::BI__builtin_rotateleft32: | ||
| case Builtin::BI__builtin_rotateleft64: | ||
| case Builtin::BI_rotl8: // Microsoft variants of rotate right | ||
| case Builtin::BI_rotl16: | ||
| case Builtin::BI_rotl: | ||
| case Builtin::BI_lrotl: | ||
| case Builtin::BI_rotl64: { | ||
| APSInt Val, Amt; | ||
| if (!EvaluateInteger(E->getArg(0), Val, Info) || | ||
| !EvaluateInteger(E->getArg(1), Amt, Info)) | ||
| return false; | ||
|
|
||
| return Success(Val.rotl(Amt), E); | ||
| } | ||
|
|
||
| case Builtin::BI__builtin_rotateright8: | ||
| case Builtin::BI__builtin_rotateright16: | ||
| case Builtin::BI__builtin_rotateright32: | ||
| case Builtin::BI__builtin_rotateright64: | ||
| case Builtin::BI__builtin_stdc_rotate_left: | ||
| case Builtin::BI__builtin_stdc_rotate_right: | ||
| case Builtin::BI_rotl8: // Microsoft variants of rotate left | ||
| case Builtin::BI_rotl16: | ||
| case Builtin::BI_rotl: | ||
| case Builtin::BI_lrotl: | ||
| case Builtin::BI_rotl64: | ||
|
Comment on lines
+14727
to
+14733
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are changing the behavior of existing builtins, is that intended? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i thought these generics will also use (unsigned, unsigned), but they evolved during the review. Those functions will still work as expected. I understand the concern (now that we are allowing more types) please let me know if we have to separate them out. |
||
| case Builtin::BI_rotr8: // Microsoft variants of rotate right | ||
| case Builtin::BI_rotr16: | ||
| case Builtin::BI_rotr: | ||
|
|
@@ -14747,7 +14741,45 @@ bool IntExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E, | |
| !EvaluateInteger(E->getArg(1), Amt, Info)) | ||
| return false; | ||
|
|
||
| return Success(Val.rotr(Amt), E); | ||
| // Normalize shift amount to [0, BitWidth) range to match runtime behavior | ||
| unsigned BitWidth = Val.getBitWidth(); | ||
| unsigned AmtBitWidth = Amt.getBitWidth(); | ||
| if (BitWidth == 1) { | ||
| // if BitWidth is 1, Amt % Divisor = 0 | ||
| // No need for rotation | ||
| Amt = APSInt(APInt(AmtBitWidth, 0), Amt.isUnsigned()); | ||
| } else { | ||
| APSInt Divisor; | ||
| if (AmtBitWidth > BitWidth) { | ||
| Divisor = APSInt(llvm::APInt(AmtBitWidth, BitWidth), Amt.isUnsigned()); | ||
| } else { | ||
| Divisor = APSInt(llvm::APInt(BitWidth, BitWidth), Amt.isUnsigned()); | ||
| if (AmtBitWidth < BitWidth) { | ||
| Amt = Amt.extend(BitWidth); | ||
| } | ||
| } | ||
|
|
||
| Amt = Amt % Divisor; | ||
| if (Amt.isNegative()) { | ||
| Amt += Divisor; | ||
| } | ||
| } | ||
|
|
||
| switch (BuiltinOp) { | ||
chaitanyav marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| case Builtin::BI__builtin_rotateright8: | ||
| case Builtin::BI__builtin_rotateright16: | ||
| case Builtin::BI__builtin_rotateright32: | ||
| case Builtin::BI__builtin_rotateright64: | ||
| case Builtin::BI__builtin_stdc_rotate_right: | ||
| case Builtin::BI_rotr8: | ||
| case Builtin::BI_rotr16: | ||
| case Builtin::BI_rotr: | ||
| case Builtin::BI_lrotr: | ||
| case Builtin::BI_rotr64: | ||
| return Success(APSInt(Val.rotr(Amt.getZExtValue()), Val.isUnsigned()), E); | ||
| default: | ||
| return Success(APSInt(Val.rotl(Amt.getZExtValue()), Val.isUnsigned()), E); | ||
| } | ||
| } | ||
|
|
||
| case Builtin::BI__builtin_elementwise_add_sat: { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pinskia Did these name ship in gcc already? the "stdc" is a bit novel and does not add much
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it shipped with GCC 15.1.0: https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Bit-Operation-Builtins.html#index-_005f_005fbuiltin_005fstdc_005frotate_005fleft
stdc is there because that are the function names in specified in C23's stdbit.h header. GCC just added _builtin in front of them here. The GCC builtins in this case is supposed to be exactly the same constaints as the C23's builtins except for allowing for
any unsigned integer (standard, extended or bit-precise).