This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add __builtin_bswap128
AbandonedPublic

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

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.EditedJan 2 2022, 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.Jan 2 2022, 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
2934

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.Jan 3 2022, 10:12 AM
clang/lib/CodeGen/CGBuiltin.cpp
2934

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

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

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.Jan 6 2022, 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?).

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?).

Where should I put the tests?

philnik updated this revision to Diff 409607.Feb 17 2022, 5:26 AM
  • Rebased
  • Address 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 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?).

Where should I put the tests?

test/CodeGen

Test with -emit-llvm. There are other similar tests :)

erichkeane added inline comments.Feb 17 2022, 6:05 AM
clang/lib/AST/ASTContext.cpp
10986–10990

Please add a test in Sema as well to validate the diagnostic so we can ensure it sounds sane.

philnik updated this revision to Diff 409629.Feb 17 2022, 6:38 AM
philnik marked an inline comment as done.
  • Add tests
xbolva00 added inline comments.Feb 25 2022, 4:03 AM
clang/docs/ReleaseNotes.rst
64

?

Wrong rebase?

philnik updated this revision to Diff 411600.Feb 26 2022, 4:51 AM
philnik marked an inline comment as done.
  • Fix rebase
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 3:23 PM
aaron.ballman added inline comments.Mar 3 2022, 1:50 PM
clang/lib/AST/ASTContext.cpp
11071–11072

Why is this change needed? We don't seem to make a vector of __int128 as part of this patch, so I thought we wouldn't need the extra AllowInt128 parameter to the function.

rsmith added inline comments.Mar 3 2022, 2:04 PM
clang/lib/AST/ASTContext.cpp
11071–11072

It's also a little hard to imagine wanting to support a target that has vectors of int128 but doesn't have int128. (If the target supports a vector of int128, why not use that to implement int128?) And there are ways to extract the element type from a vector type, so exposing such a vector type in a builtin would mean we expose non-vector int128 too.

clang/test/CodeGen/builtin-bswap128.c
1

This test needs a target to be specified; it will fail if run on a target that doesn't support int128.

7–8

You don't seem to have any test coverage for code generation of the bswap intrinsic; this is only testing the constant-evaluation path. I'd like to also see a test that covers a non-constant argument and ensures we call the appropriate LLVM intrinsic.

philnik abandoned this revision.Sep 21 2023, 1:24 AM

I don't plan to work on this any time soon, so I'll abandon it to clean up the review queue.