[clang] Add __builtin_bswap128

[philnik]

Summary

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

[philnik]

ping

[philnik]

ping

[xbolva00]

Please update Release Notes

[philnik]

Is that enough or do you want something more?

[RKSimon]

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

[philnik]

Added constexpr test

[xbolva00]

Looks fine.

[RKSimon]

clang/test/Sema/constant-builtins-2.c

219

#if defined(__SIZEOF_INT128__)

?

[philnik]

Guarding test now

[craig.topper]

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?

[philnik]

In D114425#3209794, @craig.topper wrote:

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]

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.

[philnik]

In D114425#3216233, @xbolva00 wrote:

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.

[craig.topper]

In D114425#3216231, @philnik wrote:

In D114425#3209794, @craig.topper wrote:

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.

[• Quuxplusone]

In D114425#3216490, @craig.topper wrote:

In D114425#3216231, @philnik wrote:

In D114425#3209794, @craig.topper wrote:

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

[craig.topper]

In D114425#3216531, @Quuxplusone wrote:

In D114425#3216490, @craig.topper wrote:

In D114425#3216231, @philnik wrote:

In D114425#3209794, @craig.topper wrote:

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]

• Rebased
• Error if __int128 is not supported

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

[majnemer]

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.

[• Quuxplusone]

In D114425#3216802, @majnemer wrote:

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]

clang/lib/CodeGen/CGBuiltin.cpp

2930

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

[craig.topper]

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.

[majnemer]

In D114425#3217478, @Quuxplusone wrote:

In D114425#3216802, @majnemer wrote:

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]

• Addressed comments

[craig.topper]

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

[erichkeane]

In D114425#3225892, @craig.topper wrote:

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

[philnik]

In D114425#3225908, @erichkeane wrote:

In D114425#3225892, @craig.topper wrote:

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]

• Rebased
• Address comments

[xbolva00]

In D114425#3329303, @philnik wrote:

In D114425#3225908, @erichkeane wrote:

In D114425#3225892, @craig.topper wrote:

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]

clang/lib/AST/ASTContext.cpp

10987 ↗

(On Diff #409607)

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

[philnik]

• Add tests

[philnik]

ping

[xbolva00]

clang/docs/ReleaseNotes.rst

65

?

Wrong rebase?

[philnik]

• Fix rebase

[philnik]

ping

[aaron.ballman]

clang/lib/AST/ASTContext.cpp

11071–11072 ↗

(On Diff #411600)

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]

clang/lib/AST/ASTContext.cpp

11071–11072 ↗

(On Diff #411600)

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 ↗

(On Diff #411600)

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

7–8 ↗

(On Diff #411600)

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]

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

clang/docs/ReleaseNotes.rst

- Added diagnostic groups to control diagnostics for attribute extensions by
   adding groups ``-Wc++N-attribute-extensions`` (where ``N`` is the standard
   release being diagnosed against). These new groups are automatically implied
   when passing ``-Wc++N-extensions``. Resolves PR33518.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 - Maximum _ExtInt size was decreased from 16,777,215 bits to 8,388,608 bits.
   Motivation for this was discussed in PR51829.
 - Configuration file syntax extended with ``<CFGDIR>`` token. This expands to
   the base path of the current config file. See :ref:`configuration-files` for
   details.
 
+- The builtin ``__builtin_bswap128`` was added.
+
 New Compiler Flags
 ------------------
 
 - Clang plugin arguments can now be passed through the compiler driver via
   ``-fplugin-arg-pluginname-arg``, similar to GCC's ``-fplugin-arg``.
 
 Deprecated Compiler Flags
 -------------------------

clang/include/clang/Basic/Builtins.def

 
 // The following builtins rely on that char == 8 bits, short == 16 bits and that
 // there exists native types on the target that are 32- and 64-bits wide, unless
 // these conditions are fulfilled these builtins will operate on a not intended
 // bitwidth.
 BUILTIN(__builtin_bswap16, "UsUs", "nc")
 BUILTIN(__builtin_bswap32, "UZiUZi", "nc")
 BUILTIN(__builtin_bswap64, "UWiUWi", "nc")
+BUILTIN(__builtin_bswap128, "ULLLiULLLi", "nc")
 
 BUILTIN(__builtin_bitreverse8, "UcUc", "nc")
 BUILTIN(__builtin_bitreverse16, "UsUs", "nc")
 BUILTIN(__builtin_bitreverse32, "UZiUZi", "nc")
 BUILTIN(__builtin_bitreverse64, "UWiUWi", "nc")
 
 BUILTIN(__builtin_rotateleft8, "UcUcUc", "nc")
 BUILTIN(__builtin_rotateleft16, "UsUsUs", "nc")

clang/lib/AST/ExprConstant.cpp

case Builtin::BI__builtin_bitreverse64: {
     if (!EvaluateInteger(E->getArg(0), Val, Info))
       return false;
 
     return Success(Val.reverseBits(), E);
   }
 
   case Builtin::BI__builtin_bswap16:
   case Builtin::BI__builtin_bswap32:
-  case Builtin::BI__builtin_bswap64: {
+  case Builtin::BI__builtin_bswap64:
+  case Builtin::BI__builtin_bswap128: {
     APSInt Val;
     if (!EvaluateInteger(E->getArg(0), Val, Info))
       return false;
 
     return Success(Val.byteSwap(), E);
   }
 
   case Builtin::BI__builtin_classify_type:

clang/lib/CodeGen/CGBuiltin.cpp

case Builtin::BI__arithmetic_fence: {
     Value *ArgValue = EmitScalarExpr(E->getArg(0));
     if (isArithmeticFenceEnabled)
       return RValue::get(
           Builder.CreateArithmeticFence(ArgValue, ConvertType(ArgType)));
     return RValue::get(ArgValue);
   }
   case Builtin::BI__builtin_bswap16:
   case Builtin::BI__builtin_bswap32:
-  case Builtin::BI__builtin_bswap64: {
+  case Builtin::BI__builtin_bswap64:
+  case Builtin::BI__builtin_bswap128: {
+    if (!Target.hasInt128Type() && BuiltinIDIfNoAsmLabel == Builtin::BI__builtin_bswap128)
+      CGM.ErrorUnsupported(E, "__builtin_bswap128");
     return RValue::get(emitUnaryBuiltin(*this, E, Intrinsic::bswap));
   }
   case Builtin::BI__builtin_bitreverse8:
   case Builtin::BI__builtin_bitreverse16:
   case Builtin::BI__builtin_bitreverse32:
   case Builtin::BI__builtin_bitreverse64: {
     return RValue::get(emitUnaryBuiltin(*this, E, Intrinsic::bitreverse));
   }

clang/test/CodeGen/builtins.cpp

 #include <stdint.h>
 
 extern uint16_t bswap16;
 decltype(__builtin_bswap16(0)) bswap16 = 42;
 extern uint32_t bswap32;
 decltype(__builtin_bswap32(0)) bswap32 = 42;
 extern uint64_t bswap64;
 decltype(__builtin_bswap64(0)) bswap64 = 42;
+#ifdef __SIZEOF_INT128__
+extern __uint128_t bswap128;
+decltype(__builtin_bswap128(0)) bswap128 = 42;
+#endif
 
 #ifdef __clang__
 extern uint8_t bitrev8;
 decltype(__builtin_bitreverse8(0)) bitrev8 = 42;
 extern uint16_t bitrev16;
 decltype(__builtin_bitreverse16(0)) bitrev16 = 42;
 extern uint32_t bitrev32;
 decltype(__builtin_bitreverse32(0)) bitrev32 = 42;

clang/test/Sema/constant-builtins-2.c

 extern int f();
 
 int h0 = __builtin_types_compatible_p(int, float);
 //int h1 = __builtin_choose_expr(1, 10, f());
 //int h2 = __builtin_expect(0, 0);
 int h3 = __builtin_bswap16(0x1234) == 0x3412 ? 1 : f();
 int h4 = __builtin_bswap32(0x1234) == 0x34120000 ? 1 : f();
 int h5 = __builtin_bswap64(0x1234) == 0x3412000000000000 ? 1 : f();
+#if defined(__SIZEOF_INT128__)
+int h6 = __builtin_bswap128(0x1234) == (((__int128)0x3412) << 112) ? 1 : f();
+#endif
 extern long int bi0;
 extern __typeof__(__builtin_expect(0, 0)) bi0;
 
 // Strings
 int array1[__builtin_strlen("ab\0cd")];
 int array2[(sizeof(array1)/sizeof(int)) == 2? 1 : -1];