-
Notifications
You must be signed in to change notification settings - Fork 15k
[Clang] Add __builtin_bswapg #162433
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?
[Clang] Add __builtin_bswapg #162433
Changes from 9 commits
92466f3
d4ea868
fa016a1
c0d0f3c
1e9895c
bca24f2
ba83b56
a2a6191
dfd3385
6afe334
ed8c33a
6333668
2b4e009
42301e8
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 |
|---|---|---|
|
|
@@ -3622,6 +3622,17 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID, | |
| Builder.CreateArithmeticFence(ArgValue, ConvertType(ArgType))); | ||
| return RValue::get(ArgValue); | ||
| } | ||
| case Builtin::BI__builtin_bswapg: { | ||
| Value *ArgValue = EmitScalarExpr(E->getArg(0)); | ||
| llvm::IntegerType *IntTy = cast<llvm::IntegerType>(ArgValue->getType()); | ||
| assert(IntTy && "LLVM's __builtin_bswapg only supports integer variants"); | ||
|
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. This should probably have a getBitWidth() % 8 == 0 and getBitWidth() > 0, as a defense for future support of 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. As @philnik777 described in #160266, bswapg should work on any integral types that have a multiple of 16 bits as well as a single byte. Thus, the condition should be (getBitWidth() % 16 == 0 and getBitWidth() > 0) or getBitWidth() == 8? 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 don't think mod 16 is right either, swapping Would it be more reasonable to simply say it must be a power of 2 number of bytes (N=8*2^^N for some positive N)? @philnik777 does that sound reasonable? 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. It sounds reasonable, but the llvm intrinsic has the constraint on 16 bits. I don't think we should block introducing the builtin on extending the llvm intrinsic. |
||
| assert(((IntTy->getBitWidth() % 16 == 0 && IntTy->getBitWidth() != 0) || IntTy->getBitWidth() == 8) && | ||
| "LLVM's __builtin_bswapg only supports integer variants that has a multiple of 16 bits as well as a single byte"); | ||
| if (IntTy->getBitWidth() == 8) | ||
| return RValue::get(ArgValue); | ||
| return RValue::get( | ||
| emitBuiltinWithOneOverloadedType<1>(*this, E, Intrinsic::bswap)); | ||
| } | ||
| case Builtin::BI__builtin_bswap16: | ||
| case Builtin::BI__builtin_bswap32: | ||
| case Builtin::BI__builtin_bswap64: | ||
|
|
||
clingfei marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -479,6 +479,21 @@ int h0 = __builtin_types_compatible_p(int, float); | |
| int h3 = __builtin_bswap16(0x1234) == 0x3412 ? 1 : f(); | ||
| int h4 = __builtin_bswap32(0x1234) == 0x34120000 ? 1 : f(); | ||
| int h5 = __builtin_bswap64(0x1234) == 0x3412000000000000 ? 1 : f(); | ||
| int h6 = __builtin_bswapg((char)(0x12)) == (char)(0x12) ? 1 : f(); | ||
| int h7 = __builtin_bswapg((short)(0x1234)) == (short)(0x3412) ? 1 : f(); | ||
| int h8 = __builtin_bswapg(0x00001234) == 0x34120000 ? 1 : f(); | ||
| int h9 = __builtin_bswapg(0x0000000000001234ULL) == 0x3412000000000000 ? 1 : f(); | ||
| float h10 = __builtin_bswapg(1.0f); // expected-error {{1st argument must be a scalar integer type (was 'float')}} | ||
|
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. How about also testing: arrays, pointers, enums, class, nullptr_t. What do we expect w/ charN_t, wchar_t, bool and int128_t types? 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. Testing for arrays, pointers, enums, class, nullptr_t has been added.
Currently __builtin_bswapg treats these types the same way as the corresponding byte integer types. 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. Thank you for the additional test, should we add a |
||
| double h12 = __builtin_bswapg(1.0L); // expected-error {{1st argument must be a scalar integer type (was 'long double')}} | ||
| char *h13 = __builtin_bswapg("hello"); // expected-error {{1st argument must be a scalar integer type (was 'char[6]')}} | ||
| int h14 = __builtin_bswapg(1, 2); // expected-error {{too many arguments to function call, expected 1, have 2}} | ||
| int *h15 = __builtin_bswapg(&h9); // expected-error {{1st argument must be a scalar integer type (was 'int *')}} | ||
| int arr[4] = {0x12, 0x34, 0x56, 0x78}; | ||
| int h16 = __builtin_bswapg(arr); // expected-error {{1st argument must be a scalar integer type (was 'int[4]')}} | ||
| enum BasicEnum { | ||
| ENUM_VALUE1 = 0x1234, | ||
| }; | ||
| int h17 = __builtin_bswapg(ENUM_VALUE1) == 0x34120000 ? 1 : f(); | ||
| extern long int bi0; | ||
| extern __typeof__(__builtin_expect(0, 0)) bi0; | ||
|
|
||
|
|
||
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.
Should this match the other generic ones?
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.
No, this will cause a build error:
llvm-project/clang/include/clang/Basic/Builtins.td:761:7: error: Not a template
let Prototype = "T(T)";
^
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.
IIRC
T(T)assumes an explicit set of type mappings. In principle I think this could be labeled asT(T)with the customtypechecking flag. That would be more correct thanint(..), and the Sema work is already doing the custom checking.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 wonder whether we should have a
CustomTypeCheckingBuiltinthat doesn't ask for a prototype. I don't think that would work without any modification to Clang though. Some builtins might assume the return type is correct and won't set it again during custom type checking.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 it would be nice if we could automate type checking for truly template/generic builtins. I feel like builtins using CustomTypeChecking should have an accurate prototype, but I agree that no prototype is better than a fake one.
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.
Yeah, some more infrastructure for this would be nice, especially since we seem to go in the direction of "all the builtins are generic".
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.
In fact, at first I attempted to use T(T) with an explicit set of type mappings, but the automatically generated builtin functions are bswapg16, bswapg32, etc., rather than a single generic bswapg. I'm not sure if I can use T(T) without making more modifications to clang. It would be nice if that worked.
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.
sorry, missed this reply - yeah it exists to automate the bindings for multiple types rather than just specifying a generic function :-/