Skip to content
Open
Show file tree
Hide file tree
Changes from 6 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
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
8 changes: 5 additions & 3 deletions clang/lib/AST/ByteCode/InterpBuiltin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1004,8 +1004,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 @@ -3288,7 +3290,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 @@ -13982,13 +13982,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
11 changes: 11 additions & 0 deletions clang/lib/CodeGen/CGBuiltin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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");
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
30 changes: 30 additions & 0 deletions clang/lib/Sema/SemaChecking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2200,6 +2200,32 @@ 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;
}
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 +3474,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
23 changes: 23 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,29 @@ 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();

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
5 changes: 4 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
37 changes: 37 additions & 0 deletions clang/test/CodeGenCXX/builtins.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,40 @@ 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

// 2. 不同大小的引用测试
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
8 changes: 8 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,14 @@ 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}}
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
160 changes: 160 additions & 0 deletions clang/test/SemaCXX/builtin-bswapg.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s
// RUN: %clang_cc1 -fsyntax-only -verify -fexperimental-new-constant-interpreter %s

void test_basic_type_checks() {
static_assert(__is_same(char, decltype(__builtin_bswapg((char)0))), "");
static_assert(__is_same(unsigned char, decltype(__builtin_bswapg((unsigned char)0))), "");
static_assert(__is_same(short, decltype(__builtin_bswapg((short)0))), "");
static_assert(__is_same(unsigned short, decltype(__builtin_bswapg((unsigned short)0))), "");
static_assert(__is_same(int, decltype(__builtin_bswapg((int)0))), "");
static_assert(__is_same(unsigned int, decltype(__builtin_bswapg((unsigned int)0))), "");
static_assert(__is_same(long, decltype(__builtin_bswapg((long)0))), "");
static_assert(__is_same(unsigned long, decltype(__builtin_bswapg((unsigned long)0))), "");
}

template<typename T>
void test_template_type_check() {
static_assert(__is_same(T, decltype(__builtin_bswapg(T{}))),
"bswapg should return the same type as its argument");
constexpr T zero{};
constexpr T max = ~T{};
constexpr T one = T{1};

static_assert(__is_same(T, decltype(__builtin_bswapg(zero))), "");
static_assert(__is_same(T, decltype(__builtin_bswapg(max))), "");
static_assert(__is_same(T, decltype(__builtin_bswapg(one))), "");
}
template void test_template_type_check<char>();
template void test_template_type_check<unsigned char>();
template void test_template_type_check<short>();
template void test_template_type_check<unsigned short>();
template void test_template_type_check<int>();
template void test_template_type_check<unsigned int>();
template void test_template_type_check<long>();
template void test_template_type_check<unsigned long>();

void test_lambda_type_checks() {
auto lambda = [](auto x) {
static_assert(__is_same(decltype(x), decltype(__builtin_bswapg(x))),
"bswapg in lambda should preserve type");
return __builtin_bswapg(x);
};
auto result_long = lambda(42UL);
static_assert(__is_same(unsigned long, decltype(result_long)), "");

auto result_int = lambda(42);
static_assert(__is_same(int, decltype(result_int)), "");

auto result_short = lambda(static_cast<short>(42));
static_assert(__is_same(short, decltype(result_short)), "");

auto result_char = lambda(static_cast<char>(42));
static_assert(__is_same(char, decltype(result_char)), "");
}

decltype(auto) test_decltype_auto(int x) {
return __builtin_bswapg(x);
}

void test_decltype_auto_check() {
int x = 42;
auto result = test_decltype_auto(x);
static_assert(__is_same(int, decltype(result)), "");
}

template<auto Value>
struct ValueTemplateTypeTest {
using value_type = decltype(Value);
using result_type = decltype(__builtin_bswapg(Value));

static constexpr bool type_matches = __is_same(value_type, result_type);
static_assert(type_matches, "Value template bswapg should preserve type");

static constexpr auto swapped_value = __builtin_bswapg(Value);
};

template<auto... Values>
void test_template_pack_types() {
static_assert((__is_same(decltype(Values), decltype(__builtin_bswapg(Values))) && ...), "All pack elements should preserve type");
}

template struct ValueTemplateTypeTest<0x1234>;
template struct ValueTemplateTypeTest<0x12345678UL>;
template struct ValueTemplateTypeTest<(short)0x1234>;
template struct ValueTemplateTypeTest<(char)0x12>;

template<typename T>
void test_invalid_type() {
__builtin_bswapg(T{}); // #invalid_type_use
}

void test_basic_errors() {
test_invalid_type<float>();
// expected-note@-1 {{in instantiation of function template specialization 'test_invalid_type<float>' requested here}}
// expected-error@#invalid_type_use {{1st argument must be a scalar integer type (was 'float')}}

test_invalid_type<double>();
// expected-note@-1 {{in instantiation of function template specialization 'test_invalid_type<double>' requested here}}
// expected-error@#invalid_type_use {{1st argument must be a scalar integer type (was 'double')}}

test_invalid_type<void*>();
// expected-note@-1 {{in instantiation of function template specialization 'test_invalid_type<void *>' requested here}}
// expected-error@#invalid_type_use {{1st argument must be a scalar integer type (was 'void *')}}
}

template<typename T>
auto test_dependent_context(T value) -> decltype(__builtin_bswapg(value)) { // #dependent_use
return __builtin_bswapg(value);
}

void test_dependent_errors() {
test_dependent_context(1.0f);
// expected-error@-1 {{no matching function for call to 'test_dependent_context'}}
// expected-note@#dependent_use {{candidate template ignored: substitution failure [with T = float]: 1st argument must be a scalar integer type (was 'float')}}
test_dependent_context(1.0l);
// expected-error@-1 {{no matching function for call to 'test_dependent_context'}}
// expected-note@#dependent_use {{candidate template ignored: substitution failure [with T = long double]: 1st argument must be a scalar integer type (was 'long double')}}
test_dependent_context("hello");
// expected-error@-1 {{no matching function for call to 'test_dependent_context'}}
// expected-note@#dependent_use {{candidate template ignored: substitution failure [with T = const char *]: 1st argument must be a scalar integer type (was 'const char *')}}
}

void test_lambda_errors() {
auto lambda = [](auto x) {
return __builtin_bswapg(x); // #lambda_use
};

lambda(1.0f);
// expected-error@#lambda_use {{1st argument must be a scalar integer type (was 'float')}}
// expected-note@-2 {{in instantiation of function template specialization 'test_lambda_errors()::(anonymous class)::operator()<float>' requested here}}
lambda(1.0l);
// expected-error@#lambda_use {{1st argument must be a scalar integer type (was 'long double')}}
// expected-note@-2 {{in instantiation of function template specialization 'test_lambda_errors()::(anonymous class)::operator()<long double>' requested here}}
lambda("hello");
// expected-error@#lambda_use {{1st argument must be a scalar integer type (was 'const char *')}}
// expected-note@-2 {{in instantiation of function template specialization 'test_lambda_errors()::(anonymous class)::operator()<const char *>' requested here}}
}

void test_argument_count_errors() {
int h14 = __builtin_bswapg(1, 2); // expected-error {{too many arguments to function call, expected 1, have 2}}
}

void test_lvalue_reference(int& a) {
auto result = __builtin_bswapg(a);
static_assert(__is_same(int, decltype(result)), "Should decay reference to value type");
}

void test_const_lvalue_reference(const int& a) {
auto result = __builtin_bswapg(a);
static_assert(__is_same(int, decltype(result)), "Should decay const reference to value type");
}

void test_rvalue_reference(int&& a) {
auto result = __builtin_bswapg(a);
static_assert(__is_same(int, decltype(result)), "Should decay rvalue reference to value type");
}

void test_const_rvalue_reference(const int&& a) {
auto result = __builtin_bswapg(a);
static_assert(__is_same(int, decltype(result)), "Should decay const rvalue reference to value type");
}