[PATCH] D114425: [clang] Add __builtin_bswap128

Arthur O'Dwyer via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 3 06:39:14 PST 2022


Quuxplusone added a comment.

In D114425#3216802 <https://reviews.llvm.org/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.



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2930
+  }
+  [[fallthrough]];
   case Builtin::BI__builtin_bswap16:
----------------
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));
  }
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114425/new/

https://reviews.llvm.org/D114425



More information about the cfe-commits mailing list