-
Notifications
You must be signed in to change notification settings - Fork 15.6k
[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
cfbcaa7
1e77d3b
e2d771e
131c42f
a2c32c9
0b9acd1
1bad33c
2969b6b
6208e47
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 |
|---|---|---|
|
|
@@ -16336,34 +16336,46 @@ 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: | ||
|
Collaborator
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. Per https://discourse.llvm.org/t/updating-expectations-for-the-new-constexpr-engine/88853, please also update clang/lib/AST/ByteCode/InterpBuiltin.cpp .
Contributor
Author
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 have added the implementation in InterpBuiltin.cpp |
||
| 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
+16343
to
+16349
Contributor
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?
Contributor
Author
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: | ||
| case Builtin::BI_lrotr: | ||
| case Builtin::BI_rotr64: { | ||
| APSInt Val, Amt; | ||
| if (!EvaluateInteger(E->getArg(0), Val, Info) || | ||
| !EvaluateInteger(E->getArg(1), Amt, Info)) | ||
| APSInt Value, Amount; | ||
| if (!EvaluateInteger(E->getArg(0), Value, Info) || | ||
| !EvaluateInteger(E->getArg(1), Amount, Info)) | ||
| return false; | ||
|
|
||
| return Success(Val.rotr(Amt), E); | ||
| Amount = NormalizeRotateAmount(Value, Amount); | ||
|
|
||
| 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(Value.rotr(Amount.getZExtValue()), Value.isUnsigned()), E); | ||
| default: | ||
| return Success( | ||
| APSInt(Value.rotl(Amount.getZExtValue()), Value.isUnsigned()), E); | ||
| } | ||
| } | ||
|
|
||
| case Builtin::BI__builtin_elementwise_add_sat: { | ||
|
|
@@ -19741,6 +19753,46 @@ void HandleComplexComplexDiv(APFloat A, APFloat B, APFloat C, APFloat D, | |
| } | ||
| } | ||
|
|
||
| APSInt NormalizeRotateAmount(const APSInt &Value, const APSInt &Amount) { | ||
| // Normalize shift amount to [0, BitWidth) range to match runtime behavior | ||
| APSInt NormAmt = Amount; | ||
| unsigned BitWidth = Value.getBitWidth(); | ||
| unsigned AmtBitWidth = NormAmt.getBitWidth(); | ||
| if (BitWidth == 1) { | ||
| // Rotating a 1-bit value is always a no-op | ||
| NormAmt = APSInt(APInt(AmtBitWidth, 0), NormAmt.isUnsigned()); | ||
| } else if (BitWidth == 2) { | ||
| // For 2-bit values: rotation amount is 0 or 1 based on | ||
| // whether the amount is even or odd. We can't use srem here because | ||
| // the divisor (2) would be misinterpreted as -2 in 2-bit signed arithmetic. | ||
| NormAmt = | ||
| APSInt(APInt(AmtBitWidth, NormAmt[0] ? 1 : 0), NormAmt.isUnsigned()); | ||
| } else { | ||
| APInt Divisor; | ||
| if (AmtBitWidth > BitWidth) { | ||
| Divisor = llvm::APInt(AmtBitWidth, BitWidth); | ||
| } else { | ||
| Divisor = llvm::APInt(BitWidth, BitWidth); | ||
| if (AmtBitWidth < BitWidth) { | ||
| NormAmt = NormAmt.extend(BitWidth); | ||
| } | ||
| } | ||
|
|
||
| // Normalize to [0, BitWidth) | ||
| if (NormAmt.isSigned()) { | ||
| NormAmt = APSInt(NormAmt.srem(Divisor), /*isUnsigned=*/false); | ||
| if (NormAmt.isNegative()) { | ||
| APSInt SignedDivisor(Divisor, /*isUnsigned=*/false); | ||
| NormAmt += SignedDivisor; | ||
| } | ||
| } else { | ||
| NormAmt = APSInt(NormAmt.urem(Divisor), /*isUnsigned=*/true); | ||
| } | ||
| } | ||
|
|
||
| return NormAmt; | ||
| } | ||
|
|
||
| bool ComplexExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) { | ||
| if (E->isPtrMemOp() || E->isAssignmentOp() || E->getOpcode() == BO_Comma) | ||
| return ExprEvaluatorBaseTy::VisitBinaryOperator(E); | ||
|
|
||
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).