Page MenuHomePhabricator

[clang] Add __builtin_bswap128
Needs ReviewPublic

Authored by philnik on Nov 23 2021, 2:08 AM.

Details

Reviewers
rsmith
RKSimon
aaron.ballman
erichkeane
Group Reviewers
Restricted Project
Summary

GCC has a __builtin_bswap128 which is used for std::byteswap(). Clang should also add this builtin.

Diff Detail

Unit TestsFailed

TimeTest
50 msx64 debian > LLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-go go=/usr/bin/go test llvm.org/llvm/bindings/go/llvm

Event Timeline

philnik requested review of this revision.Nov 23 2021, 2:08 AM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 2:08 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
philnik added a reviewer: Restricted Project.Dec 2 2021, 4:08 PM

Please update Release Notes

philnik updated this revision to Diff 395316.Dec 19 2021, 4:09 AM

Is that enough or do you want something more?

please can you add a constexpr test? Existing bswap tests are in clang/test/Sema/constant-builtins-2.c

philnik updated this revision to Diff 395319.Dec 19 2021, 5:22 AM

Added constexpr test

RKSimon added inline comments.Dec 19 2021, 7:11 AM
clang/test/Sema/constant-builtins-2.c
219
#if defined(__SIZEOF_INT128__)

?

philnik updated this revision to Diff 395338.Dec 19 2021, 8:44 AM

Guarding test now

philnik marked an inline comment as done.Dec 19 2021, 8:44 AM

What does the builtin due if __int128 isn't supported? Even though the type isn't legal the builtin can still be called with a narrower type that would be implicitly converted. Does that work correctly?

What does the builtin due if __int128 isn't supported? Even though the type isn't legal the builtin can still be called with a narrower type that would be implicitly converted. Does that work correctly?

Would the correct behavior be to throw an error in that case? Or what exactly do you expect?

xbolva00 added a comment.EditedSun, Jan 2, 5:49 AM

int num = …;

builtin_bswap64(num); works, no error
builtin_bswap128(num); should work as well

Highly likely this case works already with ur patch, so it would be great to add a test for this scenario.

int num = …;

builtin_bswap64(num); works, no error
builtin_bswap128(num); should work as well

Highly likely this case works already with ur patch, so it would be great to add a test for this scenario.

This should already be tested, because 0x1234 is an int and should be implicitly converted to __int128.

The question was about the scenario where clang doesn't support __int128.
__builtin_bswap128() returns an unsigned __int128. This would allow someone to do weird stuff like:

using int128 = decltype(__builtin_bswap128(0));
static_assert(sizeof(int128) == 16);

This actually works with the current patch and doesn't seem like it should work.

What does the builtin due if __int128 isn't supported? Even though the type isn't legal the builtin can still be called with a narrower type that would be implicitly converted. Does that work correctly?

Would the correct behavior be to throw an error in that case? Or what exactly do you expect?

gcc only defines the builtin if __int128 is a supported type. It doesn't look like it generates an error, it just leaves it as call to an unknown function. I don't know how easy it is to do the same in clang.

What does the builtin due if __int128 isn't supported? Even though the type isn't legal the builtin can still be called with a narrower type that would be implicitly converted. Does that work correctly?

Would the correct behavior be to throw an error in that case? Or what exactly do you expect?

gcc only defines the builtin if __int128 is a supported type. It doesn't look like it generates an error, it just leaves it as call to an unknown function. I don't know how easy it is to do the same in clang.

The existing code has some lines like CGM.ErrorUnsupported(E, "__builtin_dwarf_sp_column"); — I would try doing the same kind of thing in the case where __int128 isn't supported. (But I don't know how to make __int128 unsupported! Every place I can see int128 mentioned in the code, it's not conspicuously guarded by any condition.)

What does the builtin due if __int128 isn't supported? Even though the type isn't legal the builtin can still be called with a narrower type that would be implicitly converted. Does that work correctly?

Would the correct behavior be to throw an error in that case? Or what exactly do you expect?

gcc only defines the builtin if __int128 is a supported type. It doesn't look like it generates an error, it just leaves it as call to an unknown function. I don't know how easy it is to do the same in clang.

The existing code has some lines like CGM.ErrorUnsupported(E, "__builtin_dwarf_sp_column"); — I would try doing the same kind of thing in the case where __int128 isn't supported. (But I don't know how to make __int128 unsupported! Every place I can see int128 mentioned in the code, it's not conspicuously guarded by any condition.)

I believe it controlled by hasInt128Type() in include/clang/Basic/TargetInfo.h. It should return false for riscv32 or i686 and probably other 32-bit targets.

philnik updated this revision to Diff 396960.Sun, Jan 2, 3:54 PM
  • Rebased
  • Error if __int128 is not supported

Should it be tested that the compiler errors when calling __builtin_bswap128 with __int128 not available?

OOC, how hard would it be to generalize this builtin a little? It is nice that we have builtins like __builtin_add_overflow which do the right thing regardless of their input.

It seems like it would be nice if we started to expose more intrinsics which did the right thing regardless of operand width; another bonus is that it composes well with language features like _BitInt.

OOC, how hard would it be to generalize this builtin a little? It is nice that we have builtins like __builtin_add_overflow which do the right thing regardless of their input.

It seems like it would be nice if we started to expose more intrinsics which did the right thing regardless of operand width; another bonus is that it composes well with language features like _BitInt.

IMHO such builtins are nice iff the programmer can be 100% sure that the compiler will interpret them the same way as a human reader. __builtin_add_overflow is easy because its first two arguments are "mathematical integers" (where integer promotion doesn't matter) and its third argument is a pointer (where integer promotion can't happen). So you can really throw any combination of types at it, and it'll do "the right thing" https://godbolt.org/z/sa7b894oa (although I admit I was surprised that this worked).
For a hypothetical __builtin_bswap, you would probably need a similar pointer-based interface like

short s16 = 0xFEDC;
__builtin_bswap(&s16);  // hypothetically
assert(s16 == 0xDCFE);

assert(__builtin_bswap16(s16) == 0x0000DCFE);
assert(__builtin_bswap32(s16) == 0xDCFEFFFF);  // the problem to solve: s16 eagerly promotes to int, which changes the result

The downside is that the pointer-based interface is less ergonomic than today's value-based signatures, and probably worse codegen at -O0 (because the programmer has to materialize the operand into a named variable, and then the compiler won't remove that variable because you might want to debug it). The upside (as you said) is that a generic builtin could work with _ExtInt types and so on.

clang/lib/CodeGen/CGBuiltin.cpp
2930

Re clang-format's complaint: I would either move [[fallthrough]]; inside the curly braces, or (probably better) just eliminate the fallthrough by either duplicating line 2934 or else doing

case Builtin::BI__builtin_bswap64:
case Builtin::BI__builtin_bswap128: {
  if (BuiltinIDIfNoAsmLabel == Builtin::BI__builtin_bswap128 && !Target.hasInt128Type())
    CGM.ErrorUnsupported(E, "__builtin_bswap128");
  return RValue::get(emitUnaryBuiltin(*this, E, Intrinsic::bswap));
}
craig.topper added inline comments.Mon, Jan 3, 10:12 AM
clang/lib/CodeGen/CGBuiltin.cpp
2930

I believe we are still using LLVM_FALLTHROUGH rather than [[fallthrough]]

craig.topper added inline comments.Mon, Jan 3, 10:17 AM
clang/lib/CodeGen/CGBuiltin.cpp
2930

There's really no reason for the curly braces. There are no variables declared so you don't need a new scope. The code that it is being fallen into doesn't need them either.

OOC, how hard would it be to generalize this builtin a little? It is nice that we have builtins like __builtin_add_overflow which do the right thing regardless of their input.

It seems like it would be nice if we started to expose more intrinsics which did the right thing regardless of operand width; another bonus is that it composes well with language features like _BitInt.

IMHO such builtins are nice iff the programmer can be 100% sure that the compiler will interpret them the same way as a human reader. __builtin_add_overflow is easy because its first two arguments are "mathematical integers" (where integer promotion doesn't matter) and its third argument is a pointer (where integer promotion can't happen). So you can really throw any combination of types at it, and it'll do "the right thing" https://godbolt.org/z/sa7b894oa (although I admit I was surprised that this worked).
For a hypothetical __builtin_bswap, you would probably need a similar pointer-based interface like

short s16 = 0xFEDC;
__builtin_bswap(&s16);  // hypothetically
assert(s16 == 0xDCFE);

assert(__builtin_bswap16(s16) == 0x0000DCFE);
assert(__builtin_bswap32(s16) == 0xDCFEFFFF);  // the problem to solve: s16 eagerly promotes to int, which changes the result

The downside is that the pointer-based interface is less ergonomic than today's value-based signatures, and probably worse codegen at -O0 (because the programmer has to materialize the operand into a named variable, and then the compiler won't remove that variable because you might want to debug it). The upside (as you said) is that a generic builtin could work with _ExtInt types and so on.

I think one way of side stepping that problem would be to say that __builtin_bswap only works with unsigned types.

philnik updated this revision to Diff 397866.Thu, Jan 6, 6:06 AM
philnik marked 3 inline comments as done.
  • Addressed comments

I kind of wonder if we should detect the __int128 type being requested in ASTContext::GetBuiltinType and return an error up to Sema::LazilyCreateBuiltin. Probably requires a new error code and handling for it in LazilyCreateBuiltin. I assume that would catch the bad builtin earlier. As it stands now we'd still allow it if it constant folds away in ExprConstant.cpp so that it never reaches CGBuiltin.cpp. But I'm not a frontend expert. Adding more potential reviewers

I kind of wonder if we should detect the __int128 type being requested in ASTContext::GetBuiltinType and return an error up to Sema::LazilyCreateBuiltin. Probably requires a new error code and handling for it in LazilyCreateBuiltin. I assume that would catch the bad builtin earlier. As it stands now we'd still allow it if it constant folds away in ExprConstant.cpp so that it never reaches CGBuiltin.cpp. But I'm not a frontend expert. Adding more potential reviewers

I think I like that idea, "DecodeTypeFromStr" should probably test if __int128 is supported. Having us only diagnose in codegen is the wrong approach here, we need to reject it in Sema.

I would also want to see some IR-tests to show how this looks in IR, particularly one where it takes an __int128 as parameter (plus others that show the return value is valid?).