Skip to content
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,9 @@ Non-comprehensive list of changes in this release
allocator-level heap organization strategies. A feature to instrument all
allocation functions with a token ID can be enabled via the
``-fsanitize=alloc-token`` flag.

- A new generic byte swap builtin function ``__builtin_bswapg`` that extends the existing
__builtin_bswap{16,32,64} function family to support all standard integer types.

New Compiler Flags
------------------
Expand Down
6 changes: 6 additions & 0 deletions clang/include/clang/Basic/Builtins.td
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,12 @@ def BSwap : Builtin, Template<["unsigned short", "uint32_t", "uint64_t"],
let Prototype = "T(T)";
}

def BSwapg : Builtin {
let Spellings = ["__builtin_bswapg"];
let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
let Prototype = "int(...)";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let Prototype = "int(...)";
let Prototype = "T(T)";

Should this match the other generic ones?

Copy link
Author

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)";
^

Copy link
Contributor

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 as T(T) with the customtypechecking flag. That would be more correct than int(..), and the Sema work is already doing the custom checking.

Copy link
Contributor

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 CustomTypeCheckingBuiltin that 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.

Copy link
Contributor

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.

Copy link
Contributor

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".

Copy link
Author

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 as T(T) with the customtypechecking flag. That would be more correct than int(..), and the Sema work is already doing the custom checking.

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.

Copy link
Contributor

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 :-/

}

def Bitreverse : BitInt8_16_32_64BuiltinsTemplate, Builtin {
let Spellings = ["__builtin_bitreverse"];
let Attributes = [NoThrow, Const, Constexpr];
Expand Down
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -12948,6 +12948,9 @@ def err_builtin_invalid_arg_type: Error<
"%plural{0:|: }3"
"%plural{[0,3]:type|:types}1 (was %4)">;

def err_bswapg_bitint_not_16bit_aligned : Error<
"_BitInt type %0 (%1 bits) must be a multiple of 16 bits for byte swapping">;

def err_builtin_trivially_relocate_invalid_arg_type: Error <
"first%select{||| and second}0 argument%select{|||s}0 to "
"'__builtin_trivially_relocate' must be"
Expand Down
8 changes: 5 additions & 3 deletions clang/lib/AST/ByteCode/InterpBuiltin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -970,8 +970,10 @@ static bool interp__builtin_bswap(InterpState &S, CodePtr OpPC,
const CallExpr *Call) {
const APSInt &Val = popToAPSInt(S, Call->getArg(0));
assert(Val.getActiveBits() <= 64);

pushInteger(S, Val.byteSwap(), Call->getType());
if (Val.getBitWidth() == 8)
pushInteger(S, Val, Call->getType());
else
pushInteger(S, Val.byteSwap(), Call->getType());
return true;
}

Expand Down Expand Up @@ -3370,7 +3372,7 @@ bool InterpretBuiltin(InterpState &S, CodePtr OpPC, const CallExpr *Call,
case Builtin::BI__builtin_elementwise_ctzg:
return interp__builtin_elementwise_countzeroes(S, OpPC, Frame, Call,
BuiltinID);

case Builtin::BI__builtin_bswapg:
case Builtin::BI__builtin_bswap16:
case Builtin::BI__builtin_bswap32:
case Builtin::BI__builtin_bswap64:
Expand Down
4 changes: 3 additions & 1 deletion clang/lib/AST/ExprConstant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14279,13 +14279,15 @@ bool IntExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E,

return Success(Val.reverseBits(), E);
}

case Builtin::BI__builtin_bswapg:
case Builtin::BI__builtin_bswap16:
case Builtin::BI__builtin_bswap32:
case Builtin::BI__builtin_bswap64: {
APSInt Val;
if (!EvaluateInteger(E->getArg(0), Val, Info))
return false;
if (Val.getBitWidth() == 8)
return Success(Val, E);

return Success(Val.byteSwap(), E);
}
Expand Down
13 changes: 13 additions & 0 deletions clang/lib/CodeGen/CGBuiltin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3622,6 +3622,19 @@ 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");
Copy link
Contributor

Choose a reason for hiding this comment

The 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 _BitInt if we don't simply include that now (which I think someone has already asked about?)

Copy link
Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think mod 16 is right either, swapping _BitInt(24) (etc) is in principle just as reasonable a swap, though it would be a novel choice. And at the same time as disallowing _BitInt(24), the mod 16 rule allows the equivalently odd _BitInt(48) to be swapped.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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:
Expand Down
38 changes: 38 additions & 0 deletions clang/lib/Sema/SemaChecking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2200,6 +2200,40 @@ static bool BuiltinCpu(Sema &S, const TargetInfo &TI, CallExpr *TheCall,
return false;
}

/// Checks that __builtin_bswapg was called with a single argument, which is an
/// unsigned integer, and overrides the return value type to the integer type.
static bool BuiltinBswapg(Sema &S, CallExpr *TheCall) {
if (S.checkArgCount(TheCall, 1))
return true;
ExprResult ArgRes = S.DefaultLvalueConversion(TheCall->getArg(0));
if (ArgRes.isInvalid())
return true;

Expr *Arg = ArgRes.get();
TheCall->setArg(0, Arg);
if (Arg->isTypeDependent())
return false;

QualType ArgTy = Arg->getType();

if (!ArgTy->isIntegerType()) {
S.Diag(Arg->getBeginLoc(), diag::err_builtin_invalid_arg_type)
<< 1 << /*scalar=*/ 1 << /*unsigned integer=*/ 1 << /*floating point=*/ 0
<< ArgTy;
return true;
}
const auto *bitIntType = dyn_cast<BitIntType>(ArgTy);
if (bitIntType != nullptr) {
if (bitIntType->getNumBits() % 16 != 0 && bitIntType->getNumBits() != 8) {
S.Diag(Arg->getBeginLoc(), diag::err_bswapg_bitint_not_16bit_aligned)
<< ArgTy << bitIntType->getNumBits();
return true;
}
}
TheCall->setType(ArgTy);
return false;
}

/// Checks that __builtin_popcountg was called with a single argument, which is
/// an unsigned integer.
static bool BuiltinPopcountg(Sema &S, CallExpr *TheCall) {
Expand Down Expand Up @@ -3448,6 +3482,10 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
}
break;
}
case Builtin::BI__builtin_bswapg:
if (BuiltinBswapg(*this, TheCall))
return ExprError();
break;
case Builtin::BI__builtin_popcountg:
if (BuiltinPopcountg(*this, TheCall))
return ExprError();
Expand Down
33 changes: 33 additions & 0 deletions clang/test/AST/ByteCode/builtin-functions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,39 @@ namespace bswap {
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(0x12) == 0x12 ? 1 : f();
int h7 = __builtin_bswapg(0x1234) == 0x3412 ? 1 : f();
int h8 = __builtin_bswapg(0x00001234) == 0x34120000 ? 1 : f();
int h9 = __builtin_bswapg(0x0000000000001234) == 0x3412000000000000 ? 1 : f();
#ifndef __AVR__
int h10 = __builtin_bswapg((_BitInt(8))0x12) == (_BitInt(8))0x12 ? 1 : f();
int h11 = __builtin_bswapg((_BitInt(16))0x1234) == (_BitInt(16))0x3412 ? 1 : f();
int h12 = __builtin_bswapg((_BitInt(32))0x00001234) == (_BitInt(32))0x34120000 ? 1 : f();
int h13 = __builtin_bswapg((_BitInt(64))0x0000000000001234) == (_BitInt(64))0x3412000000000000 ? 1 : f();
int h14 = __builtin_bswapg(~(_BitInt(128))0) == (~(_BitInt(128))0) ? 1 : f();
int h15 = __builtin_bswapg((_BitInt(24))0x1234) == (_BitInt(24))0x3412 ? 1 : f();
// expected-error@-1 {{_BitInt type '_BitInt(24)' (24 bits) must be a multiple of 16 bits for byte swapping}}
// ref-error@-2 {{_BitInt type '_BitInt(24)' (24 bits) must be a multiple of 16 bits for byte swapping}}
#endif

constexpr const int const_expr = 0x1234;

void test_constexpr_reference() {
const int expr = 0x1234;
const int& ref = expr; // #declare

constexpr const int& const_ref = const_expr;

constexpr auto result2 = __builtin_bswapg(ref);
//expected-error@-1 {{constexpr variable 'result2' must be initialized by a constant expression}}
//expected-note@-2 {{initializer of 'ref' is not a constant expression}}
//expected-note@#declare {{declared here}}
//ref-error@-4 {{constexpr variable 'result2' must be initialized by a constant expression}}
//ref-note@-5 {{initializer of 'ref' is not a constant expression}}
//ref-note@#declare {{declared here}}

constexpr auto result3 = __builtin_bswapg(const_ref);
}
}

#define CFSTR __builtin___CFStringMakeConstantString
Expand Down
37 changes: 36 additions & 1 deletion clang/test/CodeGen/builtins.c
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,10 @@ int main(void) {
P(object_size, (s0, 3));

// Whatever

P(bswapg, ((char)N));
P(bswapg, ((short)N));
P(bswapg, ((int)N));
P(bswapg, ((unsigned long)N));
P(bswap16, (N));
P(bswap32, (N));
P(bswap64, (N));
Expand Down Expand Up @@ -1277,3 +1280,35 @@ void test_builtin_ctzg(unsigned char uc, unsigned short us, unsigned int ui,
}

#endif

// CHECK-LABEL: define{{.*}} void @test_builtin_bswapg
void test_builtin_bswapg(unsigned char uc, unsigned short us, unsigned int ui,
unsigned long ul, unsigned long long ull,
unsigned __int128 ui128, _BitInt(8) bi8,
_BitInt(16) bi16, _BitInt(32) bi32,
_BitInt(64) bi64, _BitInt(128) bi128) {
uc = __builtin_bswapg(uc);
// CHECK: %1 = load i8, ptr %uc.addr
// CHECK: store i8 %1, ptr %uc.addr
us = __builtin_bswapg(us);
// CHECK: call i16 @llvm.bswap.i16
ui = __builtin_bswapg(ui);
// CHECK: call i32 @llvm.bswap.i32
ul = __builtin_bswapg(ul);
// CHECK: call i64 @llvm.bswap.i64
ull = __builtin_bswapg(ull);
// CHECK: call i64 @llvm.bswap.i64
ui128 = __builtin_bswapg(ui128);
// CHECK: call i128 @llvm.bswap.i128
bi8 = __builtin_bswapg(bi8);
// CHECK: %17 = load i8, ptr %bi8.addr, align 1
// CHECK: store i8 %17, ptr %bi8.addr
bi16 = __builtin_bswapg(bi16);
// CHECK: call i16 @llvm.bswap.i16
bi32 = __builtin_bswapg(bi32);
// CHECK: call i32 @llvm.bswap.i32
bi64 = __builtin_bswapg(bi64);
// CHECK: call i64 @llvm.bswap.i64
bi128 = __builtin_bswapg(bi128);
// CHECK: call i128 @llvm.bswap.i128
}
56 changes: 56 additions & 0 deletions clang/test/CodeGenCXX/builtins.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,59 @@ int structured_binding_size() {
return __builtin_structured_binding_size(S2);
// CHECK: ret i32 2
}

void test_int_reference(int& a) {
__builtin_bswapg(a);
}
// CHECK-LABEL: @_Z18test_int_referenceRi
// CHECK: store ptr %a, ptr
// CHECK: load ptr, ptr
// CHECK: load i32, ptr
// CHECK: call i32 @llvm.bswap.i32

void test_long_reference(long& a) {
__builtin_bswapg(a);
}
// CHECK-LABEL: @_Z19test_long_referenceRl
// CHECK: store ptr %a, ptr
// CHECK: load ptr, ptr
// CHECK: load i64, ptr
// CHECK: call i64 @llvm.bswap.i64

void test_short_reference(short& a) {
__builtin_bswapg(a);
}
// CHECK-LABEL: @_Z20test_short_referenceRs
// CHECK: store ptr %a, ptr
// CHECK: load ptr, ptr
// CHECK: load i16, ptr
// CHECK: call i16 @llvm.bswap.i16

void test_char_reference(char& a) {
__builtin_bswapg(a);
}
// CHECK-LABEL: @_Z19test_char_referenceRc
// CHECK: store ptr %a, ptr
// CHECK: load ptr, ptr
// CHECK-NOT: call i8 @llvm.bswap.i8
// CHECK: ret void

void test_bitint() {
_BitInt(8) a = 0x12;
__builtin_bswapg(a);
_BitInt(16) b = 0x1234;
__builtin_bswapg(b);
_BitInt(32) c = 0x00001234;
__builtin_bswapg(c);
_BitInt(64) d = 0x0000000000001234;
__builtin_bswapg(d);
_BitInt(128) e = ~(_BitInt(128))0;
__builtin_bswapg(e);
}
// CHECK-LABEL: @_Z11test_bitintv
// CHECK-NOT: call i8 @llvm.bswap.i8
// CHECK: call i16 @llvm.bswap.i16
// CHECK: call i32 @llvm.bswap.i32
// CHECK: call i64 @llvm.bswap.i64
// CHECK: call i128 @llvm.bswap.i128
// CHECK: ret void
22 changes: 22 additions & 0 deletions clang/test/Sema/constant-builtins-2.c
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,28 @@ 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')}}
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing for arrays, pointers, enums, class, nullptr_t has been added.

What do we expect w/ charN_t, wchar_t, bool and int128_t types?

Currently __builtin_bswapg treats these types the same way as the corresponding byte integer types.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the additional test, should we add a _BitInt test @AaronBallman

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();
int h18 = __builtin_bswapg((_BitInt(8))0x12) == (_BitInt(8))0x12 ? 1 : f();
int h19 = __builtin_bswapg((_BitInt(16))0x1234) == (_BitInt(16))0x3412 ? 1 : f();
int h20 = __builtin_bswapg((_BitInt(32))0x00001234) == (_BitInt(32))0x34120000 ? 1 : f();
int h21 = __builtin_bswapg((_BitInt(64))0x0000000000001234) == (_BitInt(64))0x3412000000000000 ? 1 : f();
int h22 = __builtin_bswapg(~(_BitInt(128))0) == (~(_BitInt(128))0) ? 1 : f();
int h23 = __builtin_bswapg((_BitInt(24))0x1234) == (_BitInt(24))0x3412 ? 1 : f();
// expected-error@-1 {{_BitInt type '_BitInt(24)' (24 bits) must be a multiple of 16 bits for byte swapping}}
extern long int bi0;
extern __typeof__(__builtin_expect(0, 0)) bi0;

Expand Down
5 changes: 4 additions & 1 deletion clang/test/Sema/constant-builtins.c
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@ 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();
short somefunc(void);

short t = __builtin_constant_p(5353) ? 42 : somefunc();
Expand Down
Loading
Loading