-
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?
Conversation
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: NagaChaitanya Vellanki (chaitanyav) ChangesResolves #122819 Full diff: https://github.com/llvm/llvm-project/pull/160259.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index 35d2c3e19fdf9..7044b48014e9d 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -767,12 +767,24 @@ def RotateLeft : BitInt8_16_32_64BuiltinsTemplate, Builtin {
let Prototype = "T(T, T)";
}
+def RotateLeftg : Builtin {
+ let Spellings = ["__builtin_rotateleftg"];
+ let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
+ let Prototype = "void(...)";
+}
+
def RotateRight : BitInt8_16_32_64BuiltinsTemplate, Builtin {
let Spellings = ["__builtin_rotateright"];
let Attributes = [NoThrow, Const, Constexpr];
let Prototype = "T(T, T)";
}
+def RotateRightg : Builtin {
+ let Spellings = ["__builtin_rotaterightg"];
+ let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
+ let Prototype = "void(...)";
+}
+
// Random GCC builtins
// FIXME: The builtins marked FunctionWithBuiltinPrefix below should be
// merged with the library definitions. They are currently not because
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index f7c3dea257d50..d5e107c91854e 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -3642,6 +3642,7 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
case Builtin::BI__builtin_rotateleft16:
case Builtin::BI__builtin_rotateleft32:
case Builtin::BI__builtin_rotateleft64:
+ case Builtin::BI__builtin_rotateleftg:
case Builtin::BI_rotl8: // Microsoft variants of rotate left
case Builtin::BI_rotl16:
case Builtin::BI_rotl:
@@ -3653,6 +3654,7 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
case Builtin::BI__builtin_rotateright16:
case Builtin::BI__builtin_rotateright32:
case Builtin::BI__builtin_rotateright64:
+ case Builtin::BI__builtin_rotaterightg:
case Builtin::BI_rotr8: // Microsoft variants of rotate right
case Builtin::BI_rotr16:
case Builtin::BI_rotr:
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 740b472b0eb16..959f7b055149f 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -2267,6 +2267,48 @@ static bool BuiltinCountZeroBitsGeneric(Sema &S, CallExpr *TheCall) {
return false;
}
+/// Checks that __builtin_{rotateleftg,rotaterightg} was called with two
+/// arguments, that the first argument is an unsigned integer type, and that
+/// the second argument is an integer type.
+static bool BuiltinRotateGeneric(Sema &S, CallExpr *TheCall) {
+ if (S.checkArgCount(TheCall, 2))
+ return true;
+
+ ExprResult Arg0Res = S.DefaultLvalueConversion(TheCall->getArg(0));
+ if (Arg0Res.isInvalid())
+ return true;
+
+ Expr *Arg0 = Arg0Res.get();
+ TheCall->setArg(0, Arg0);
+
+ QualType Arg0Ty = Arg0->getType();
+
+ if (!Arg0Ty->isUnsignedIntegerType()) {
+ S.Diag(Arg0->getBeginLoc(), diag::err_builtin_invalid_arg_type)
+ << 1 << /* scalar */ 1 << /* unsigned integer ty */ 3 << /* no fp */ 0
+ << Arg0Ty;
+ return true;
+ }
+
+ ExprResult Arg1Res = S.DefaultLvalueConversion(TheCall->getArg(1));
+ if (Arg1Res.isInvalid())
+ return true;
+
+ Expr *Arg1 = Arg1Res.get();
+ TheCall->setArg(1, Arg1);
+
+ QualType Arg1Ty = Arg1->getType();
+
+ if (!Arg1Ty->isIntegerType()) {
+ S.Diag(Arg1->getBeginLoc(), diag::err_builtin_invalid_arg_type)
+ << 2 << /* scalar */ 1 << /* integer ty */ 2 << /* no fp */ 0 << Arg1Ty;
+ return true;
+ }
+
+ TheCall->setType(Arg0Ty);
+ return false;
+}
+
static bool CheckMaskedBuiltinArgs(Sema &S, Expr *MaskArg, Expr *PtrArg,
unsigned Pos, bool Vector = true) {
QualType MaskTy = MaskArg->getType();
@@ -3416,6 +3458,12 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
return ExprError();
break;
+ case Builtin::BI__builtin_rotateleftg:
+ case Builtin::BI__builtin_rotaterightg:
+ if (BuiltinRotateGeneric(*this, TheCall))
+ return ExprError();
+ break;
+
case Builtin::BI__builtin_allow_runtime_check: {
Expr *Arg = TheCall->getArg(0);
// Check if the argument is a string literal.
diff --git a/clang/test/CodeGen/builtin-rotate.c b/clang/test/CodeGen/builtin-rotate.c
index 8fc1701c6c9bb..7a0343158f1ba 100644
--- a/clang/test/CodeGen/builtin-rotate.c
+++ b/clang/test/CodeGen/builtin-rotate.c
@@ -32,6 +32,43 @@ unsigned long long rotl64(unsigned long long x, long long y) {
return __builtin_rotateleft64(x, y);
}
+// CHECK-LABEL: test_builtin_rotateleftg
+void test_builtin_rotateleftg(unsigned char uc, unsigned short us,
+ unsigned int ui, unsigned long ul,
+ unsigned long long ull, unsigned __int128 ui128,
+ unsigned _BitInt(128) ubi128) {
+
+volatile unsigned char result_uc;
+volatile unsigned int result_ui;
+volatile unsigned short result_us;
+volatile unsigned long result_ul;
+volatile unsigned long long result_ull;
+volatile unsigned __int128 result_ui128;
+volatile unsigned _BitInt(128) result_ubi128;
+
+ // CHECK: call i8 @llvm.fshl.i8(i8 %{{.*}}, i8 %{{.*}}, i8 3)
+ result_uc = __builtin_rotateleftg(uc, 3);
+
+ // CHECK: call i16 @llvm.fshl.i16(i16 %{{.*}}, i16 %{{.*}}, i16 5)
+ result_us = __builtin_rotateleftg(us, 5);
+
+ // CHECK: call i32 @llvm.fshl.i32(i32 %{{.*}}, i32 %{{.*}}, i32 8)
+ result_ui = __builtin_rotateleftg(ui, 8);
+
+ // CHECK: call i64 @llvm.fshl.i64(i64 %{{.*}}, i64 %{{.*}}, i64 8)
+ result_ul = __builtin_rotateleftg(ul, 8);
+
+ // CHECK: call i64 @llvm.fshl.i64(i64 %{{.*}}, i64 %{{.*}}, i64 16)
+ result_ull = __builtin_rotateleftg(ull, 16);
+
+ // CHECK: call i128 @llvm.fshl.i128(i128 %{{.*}}, i128 %{{.*}}, i128 32)
+ result_ui128 = __builtin_rotateleftg(ui128, 32);
+
+ // CHECK: call i128 @llvm.fshl.i128(i128 %{{.*}}, i128 %{{.*}}, i128 64)
+ result_ubi128 = __builtin_rotateleftg(ubi128, 64);
+
+}
+
char rotr8(char x, char y) {
// CHECK-LABEL: rotr8
// CHECK: [[F:%.*]] = call i8 @llvm.fshr.i8(i8 [[X:%.*]], i8 [[X]], i8 [[Y:%.*]])
@@ -64,3 +101,39 @@ long long rotr64(long long x, unsigned long long y) {
return __builtin_rotateright64(x, y);
}
+// CHECK-LABEL: test_builtin_rotaterightg
+void test_builtin_rotaterightg(unsigned char uc, unsigned short us,
+ unsigned int ui, unsigned long ul,
+ unsigned long long ull, unsigned __int128 ui128,
+ unsigned _BitInt(128) ubi128) {
+
+ volatile unsigned char result_uc;
+ volatile unsigned int result_ui;
+ volatile unsigned short result_us;
+ volatile unsigned long result_ul;
+ volatile unsigned long long result_ull;
+ volatile unsigned __int128 result_ui128;
+ volatile unsigned _BitInt(128) result_ubi128;
+
+ // CHECK: call i8 @llvm.fshr.i8(i8 %{{.*}}, i8 %{{.*}}, i8 3)
+ result_uc = __builtin_rotaterightg(uc, 3);
+
+ // CHECK: call i16 @llvm.fshr.i16(i16 %{{.*}}, i16 %{{.*}}, i16 5)
+ result_us = __builtin_rotaterightg(us, 5);
+
+ // CHECK: call i32 @llvm.fshr.i32(i32 %{{.*}}, i32 %{{.*}}, i32 8)
+ result_ui = __builtin_rotaterightg(ui, 8);
+
+ // CHECK: call i64 @llvm.fshr.i64(i64 %{{.*}}, i64 %{{.*}}, i64 8)
+ result_ul = __builtin_rotaterightg(ul, 8);
+
+ // CHECK: call i64 @llvm.fshr.i64(i64 %{{.*}}, i64 %{{.*}}, i64 16)
+ result_ull = __builtin_rotaterightg(ull, 16);
+
+ // CHECK: call i128 @llvm.fshr.i128(i128 %{{.*}}, i128 %{{.*}}, i128 32)
+ result_ui128 = __builtin_rotaterightg(ui128, 32);
+
+ // CHECK: call i128 @llvm.fshr.i128(i128 %{{.*}}, i128 %{{.*}}, i128 64)
+ result_ubi128 = __builtin_rotaterightg(ubi128, 64);
+
+}
\ No newline at end of file
|
4fc9bde to
01dfddd
Compare
01dfddd to
2abeead
Compare
philnik777
left a comment
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.
Oh, you should also update the title.
2abeead to
e262508
Compare
philnik777
left a comment
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.
LGTM, but let's wait for one of the Clang people to take another look.
e262508 to
3fae000
Compare
3fae000 to
49034a4
Compare
clang/docs/LanguageExtensions.rst
Outdated
| accept any unsigned integer type, including ``_BitInt`` types. The rotation | ||
| count is treated as an unsigned value and taken modulo the bit-width of the | ||
| value being rotated. Negative rotation counts are converted to their unsigned | ||
| equivalent before the modulo operation. These builtins can be used within |
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.
Does "converted to their unsigned equivalent" mean reinterpreting the two's-complement representation as unsigned? Or is it the absolute value? Or something else? I'm confused by what you're doing here.
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.
please let me update that, i am normalizing the negative numbers to be in [0, bitwidth - 1] range.
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.
Wouldn't it make more sense to simply reject signed types?
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.
Tested it thoroughly
Testing edge cases for __builtin_stdc_rotate_{left,right}
=======================================================
Testing large _BitInt types...
_BitInt(64) rotate left by 63: PASS
_BitInt(96) rotate left by 95: PASS
_BitInt(128) rotate left by 127: PASS
Testing odd bit widths...
_BitInt(37) rotate left by 36: PASS
_BitInt(73) rotate left by 72: PASS
_BitInt(127) rotate left by 126: PASS
Testing extreme rotation amounts...
Rotate by UINT_MAX: PASS
Rotate by INT_MIN: PASS
Rotate by 10000: PASS
Testing boundary rotations...
Rotate by 0: PASS
Rotate by bit width (32): PASS
Rotate by 2x bit width (64): PASS
Rotate left by -1 vs rotate right by 1: PASS
Testing small _BitInt types...
_BitInt(1) rotate: PASS
_BitInt(2) rotate left by 1: PASS
_BitInt(3) rotate left by 2: PASS
Testing pattern verification...
Alternating pattern rotate: PASS
All-ones pattern rotate: PASS
Single bit walking: PASS
Testing left/right rotation consistency...
Left/right consistency: PASS
Left/right equivalence: PASS
Running performance stress test...
Performance stress test (10000 rotations): PASS
Testing maximum _BitInt(128) values...
_BitInt(128) high bit rotate: PASS
_BitInt(128) lower bits rotate: PASS
Testing zero and minimal bit cases...
Large shift on small type: PASS
Negative rotation on small type: PASS
=======================================================
All tests completed.__BitInt tests
Testing both compile-time (static assertions) and runtime paths
Static assertions passed - compile-time evaluation works!
=== Runtime Edge Case Tests ===
PASS: Large shift: 341 << 1000
PASS: Large negative: 341 << -1000
PASS: Max value rotate
PASS: Zero rotate
=== Different Bit Widths Runtime Tests ===
PASS: 3-bit rotate
PASS: 5-bit rotate
PASS: 7-bit rotate
PASS: 10-bit rotate
PASS: 13-bit rotate
=== Comprehensive Negative Shift Tests ===
rotate_left(341, -1) = 426
rotate_left(341, -2) = 213
rotate_left(341, -3) = 362
rotate_left(341, -4) = 181
rotate_left(341, -5) = 346
rotate_left(341, -6) = 173
rotate_left(341, -7) = 342
rotate_left(341, -8) = 171
rotate_left(341, -9) = 341
rotate_left(341, -10) = 426
rotate_left(341, -11) = 213
rotate_left(341, -12) = 362
rotate_left(341, -13) = 181
rotate_left(341, -14) = 346
rotate_left(341, -15) = 173
rotate_left(341, -16) = 342
rotate_left(341, -17) = 171
rotate_left(341, -18) = 341
rotate_left(341, -19) = 426
rotate_left(341, -20) = 213
=== Comprehensive Modulo Reduction Tests ===
rotate_left(341, 100) = 171 (reduced: 1)
rotate_left(341, 1000) = 171 (reduced: 1)
rotate_left(341, 10000) = 171 (reduced: 1)
rotate_left(341, 50000) = 181 (reduced: 5)
rotate_left(341, 1073741823) = 341 (reduced: 0)
=== Cross-Direction Equivalence Tests ===
rotate_left(341, -1) = 426, rotate_right(341, 1) = 426
rotate_left(341, -2) = 213, rotate_right(341, 2) = 213
rotate_left(341, -3) = 362, rotate_right(341, 3) = 362
rotate_left(341, -4) = 181, rotate_right(341, 4) = 181
rotate_left(341, -5) = 346, rotate_right(341, 5) = 346
rotate_left(341, -6) = 173, rotate_right(341, 6) = 173
rotate_left(341, -7) = 342, rotate_right(341, 7) = 342
rotate_left(341, -8) = 171, rotate_right(341, 8) = 171
rotate_left(341, -9) = 341, rotate_right(341, 9) = 341
rotate_left(341, -10) = 426, rotate_right(341, 10) = 426
rotate_left(341, -11) = 213, rotate_right(341, 11) = 213
rotate_left(341, -12) = 362, rotate_right(341, 12) = 362
rotate_left(341, -13) = 181, rotate_right(341, 13) = 181
rotate_left(341, -14) = 346, rotate_right(341, 14) = 346
rotate_left(341, -15) = 173, rotate_right(341, 15) = 173
rotate_left(341, -16) = 342, rotate_right(341, 16) = 342
rotate_left(341, -17) = 171, rotate_right(341, 17) = 171
rotate_left(341, -18) = 341, rotate_right(341, 18) = 341
rotate_left(341, -19) = 426, rotate_right(341, 19) = 426
rotate_left(341, -20) = 213, rotate_right(341, 20) = 213
=== Large Bit Width Stress Tests ===
PASS: 37-bit large value
PASS: 61-bit large value
37-bit rotate by 10000: result computed
=== Large Shifts on Small Bit Widths ===
PASS: 8-bit by 64
PASS: 8-bit by 65
PASS: 8-bit by 1000
PASS: 3-bit by 64
PASS: 3-bit by 1000
PASS: 16-bit by 65536
PASS: 16-bit by 65537
PASS: 8-bit by -64
PASS: 8-bit by -1000
Testing shift amounts much larger than bit widths: PASSED
🎉 ALL COMPREHENSIVE TESTS PASSED! 🎉
Both compile-time and runtime rotation implementations are correct!
Tested bit widths: 3, 5, 6, 7, 8, 9, 10, 11, 12, 13, 16, 37, 61
Tested shift ranges: -50000 to +65537
Tested edge cases: zero, max values, power-of-2, boundary conditions49034a4 to
deb648b
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
db50769 to
6c5c34c
Compare
24cf397 to
258cb5d
Compare
| let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking]; | ||
| let Prototype = "void(...)"; | ||
| } | ||
|
|
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
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).
| 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: |
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.
We are changing the behavior of existing builtins, is that intended?
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.
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.
|
@efriedma-quic is the only one I can see that has open comments? I'd give him time on that, otherwise I would suspect this could be merged. |
clang/lib/AST/ExprConstant.cpp
Outdated
| if (AmtBitWidth > BitWidth) { | ||
| Divisor = APSInt(llvm::APInt(AmtBitWidth, BitWidth), Amt.isUnsigned()); | ||
| } else { | ||
| Divisor = APSInt(llvm::APInt(BitWidth, BitWidth), Amt.isUnsigned()); |
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.
There's an edge case here if BitWidth is 2, and Amt is signed -1: you end up with Divisor == -2, and Amt ends up signed -3. I think you end with the right result eventually, but it's a bit hard to reason about.
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.
Fixed the issue by making Divisor unsigned.
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
| if (!llvm::isPowerOf2_32(BitWidth) && | ||
| E->getArg(1)->getType()->isSignedIntegerType()) { | ||
| // converting BitWidth to the type of ShiftAmt | ||
| llvm::Value *BitWidthInShiftTy = ConstantInt::get(ShiftTy, BitWidth); |
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.
I think you need to sign-extend the shift amount first, in some cases.
If ShiftTy is small, ConstantInt::get(ShiftTy, BitWidth); could be zero, or some other truncated number. Which I think breaks the rest of the algorithm.
Really, I'd be happier if we could make the algorithm here more closely match the algorithm in ExprConstant; having different algorithms means they have different edge cases, which makes it harder to review.
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.
Made the implementations similar to ExprConstant.cpp.
| case Builtin::BI__builtin_rotateright32: | ||
| case Builtin::BI__builtin_rotateright64: | ||
| case Builtin::BI__builtin_stdc_rotate_left: | ||
| case Builtin::BI__builtin_stdc_rotate_right: |
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.
Per https://discourse.llvm.org/t/updating-expectations-for-the-new-constexpr-engine/88853, please also update clang/lib/AST/ByteCode/InterpBuiltin.cpp .
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.
I have added the implementation in InterpBuiltin.cpp
06e28c2 to
cace5aa
Compare
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
|
@efriedma-quic addressed the feedback, please review. |
clang/lib/Sema/SemaChecking.cpp
Outdated
| QualType Arg0Ty = Arg0->getType(); | ||
| if (!Arg0Ty->isUnsignedIntegerType()) { | ||
| ExprResult ConvArg0Res = S.PerformImplicitConversion( | ||
| TheCall->getArg(0), S.Context.UnsignedIntTy, AssignmentAction::Passing); |
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.
Implicitly choosing unsigned int as the type of the argument seems like a bad idea; the value might not actually fit into an unsigned int, and there could be other weird effects if you're implicitly converting from signed to unsigned. Users can always explicitly cast if they need to.
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.
this will break support for class types with conversion operators
struct UnsignedWrapper {
operator unsigned int() const { return 42; }
};
auto result1 = __builtin_stdc_rotate_left(uw, 3);# | File /home/naga/llvm-project/clang/test/SemaCXX/constexpr-builtin-stdc-rotate.cpp Line 182: 1st argument must be a scalar unsigned integer type (was 'UnsignedWrapper')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.
Please advise on how to proceed.
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.
I'm not sure how @erichkeane expected you to do this. Maybe PerformContextualImplicitConversion()?
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.
used PerformContextualImplicitConversion to allow for class/struct types with conversion operators. Other types have to be explicitly casted.
| @@ -1,5 +1,7 @@ | |||
| // RUN: %clang_cc1 %s -emit-llvm -o - | FileCheck %s | |||
|
|
|||
| #include<stdint.h> | |||
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.
I think you need to specify -ffreestanding to make #include<stdint.h> portable.
clang/lib/AST/ExprConstant.cpp
Outdated
|
|
||
| return Success(Val.rotr(Amt), E); | ||
| // Normalize shift amount to [0, BitWidth) range to match runtime behavior | ||
| unsigned BitWidth = Val.getBitWidth(); |
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.
Please move the APInt code for performing the rotate into a helper shared by ExprConstant and InterpBuiltin.
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.
ExprConstant, InterBuiltin both share the same implementation now.
clang/lib/AST/ExprConstant.cpp
Outdated
| } else { | ||
| // Divisor is always unsigned to avoid misinterpreting BitWidth as | ||
| // negative in small bit widths (e.g., BitWidth=2 would be -2 if signed). | ||
| APSInt Divisor; |
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.
You can declare Divisor as an APInt; every use coerces from APSInt to APInt anyway.
cace5aa to
45105a8
Compare
clang/lib/AST/ExprConstant.cpp
Outdated
| NormAmt = APSInt(APInt(AmtBitWidth, 0), NormAmt.isUnsigned()); | ||
| } else { | ||
| // Divisor is always unsigned to avoid misinterpreting BitWidth as | ||
| // negative in small bit widths (e.g., BitWidth=2 would be -2 if signed). |
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.
Despite this comment, NormAmt.srem(Divisor) below interprets the divisor as signed.
Fixing this in the general code seems complicated, so maybe just special-case BitWidth == 2: the normalized rotation amount is either 0 or 1, which corresponds to whether the amount is even or odd.
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.
@efriedma-quic added the special case for bitwidth=2.
45105a8 to
e822b86
Compare
efriedma-quic
left a comment
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.
LGTM
e822b86 to
cd96f37
Compare
|
thank you @efriedma-quic @erichkeane for the review and guidance throughout the review. |
cd96f37 to
92e53db
Compare
|
EDIT: I found a bug in my code when I added the bytecode interpreter to the test. I made the change to - unsigned BitWidth = S.getASTContext().getTypeSize(QT);
+ unsigned BitWidth = S.getASTContext().getIntWidth(QT); |
…e_right This patch adds type-generic rotate builtins that accept any unsigned integer type. These builtins provide: - Support for all unsigned integer types, including _BitInt - Constexpr evaluation capability - Automatic normalization of rotation counts modulo the bit-width - Proper handling of negative rotation counts (converted to equivalent positive rotations in the opposite direction) - Implicit conversion support for both arguments, allowing bool, float, and types with conversion operators (not limited to record types) The builtins follow C23 naming conventions. Resolves llvm#122819
* Make divisor unsigned and unify implementations * Add implementation to InterpBuiltin.cpp
* Use APInt for Divisor * Move rotation normalization code into ExprConstShared.h
…ators. Explicit conversion must be done for all other types.
srem treats both operands as signed. In 2-bit representation 2 is interpretted as -2. Since the rotation amount is 0 or 1 in 2-bit, we can determine by checking if the amount is odd or even
it can be applied to all bit widths
* Fixed pushInteger to use getIntWidth(QT) for IntAP types clang/lib/AST/ByteCode/IntegralAP.h:79: void clang::interp::IntegralAP<false>::copy(const APInt &) [Signed = false]: Assertion `BitWidth == V.getBitWidth()' failed.
92e53db to
6208e47
Compare
|
@efriedma-quic @erichkeane please review. |
efriedma-quic
left a comment
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.
LGTM
|
Thank you @efriedma-quic. I don't have write access, please merge when you get a chance. |
This patch adds type-generic rotate builtins that accept any unsigned integer
type. These builtins provide:
positive rotations in the opposite direction)
types with conversion operators.
The builtins follow C23 naming conventions.
Resolves #122819