Skip to content
Open
Show file tree
Hide file tree
Changes from 4 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
9 changes: 9 additions & 0 deletions clang/lib/AST/ExprConstant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13982,6 +13982,15 @@ bool IntExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E,

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

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

case Builtin::BI__builtin_bswap16:
case Builtin::BI__builtin_bswap32:
Expand Down
9 changes: 9 additions & 0 deletions clang/lib/CodeGen/CGBuiltin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3622,6 +3622,15 @@ 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.

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 ty */ 1 << /* no fp */ 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
4 changes: 4 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,10 @@ 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();
}

#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
121 changes: 121 additions & 0 deletions clang/test/Sema/builtin-bswapg.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s
// RUN: %clang_cc1 -fsyntax-only -verify -fexperimental-new-constant-interpreter %s
// expected-no-diagnostics
template <class A, class B>
static constexpr bool is_same_type = false;

template <class A>
static constexpr bool is_same_type<A, A> = true;

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

template<typename T>
void test_template_type_check() {
static_assert(is_same_type<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_type<T, decltype(__builtin_bswapg(zero))>, "");
static_assert(is_same_type<T, decltype(__builtin_bswapg(max))>, "");
static_assert(is_same_type<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_type<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_type<unsigned long, decltype(result_long)>, "");

auto result_int = lambda(42);
static_assert(is_same_type<int, decltype(result_int)>, "");

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

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

auto test_auto_return_type_long(long x) {
auto result = __builtin_bswapg(x);
static_assert(is_same_type<long, decltype(result)>, "");
return result;
}

auto test_auto_return_type_int(int x) {
auto result = __builtin_bswapg(x);
static_assert(is_same_type<int, decltype(result)>, "");
return result;
}

auto test_auto_return_type_short(short x) {
auto result = __builtin_bswapg(x);
static_assert(is_same_type<short, decltype(result)>, "");
return result;
}

auto test_auto_return_type_char(char x) {
auto result = __builtin_bswapg(x);
static_assert(is_same_type<char, decltype(result)>, "");
return result;
}

void test_auto_return_type() {
test_auto_return_type_long(42);
test_auto_return_type_int(42);
test_auto_return_type_short(42);
test_auto_return_type_char(42);
}

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_type<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_type<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_type<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>;
4 changes: 4 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,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();
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