[PATCH] D114425: [clang] Add __builtin_bswap128

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:11071-11072
 
-    QualType ElementType = DecodeTypeFromStr(Str, Context, Error,
-                                             RequiresICE, false);
+    QualType ElementType =
+        DecodeTypeFromStr(Str, Context, Error, RequiresICE, false, true);
     assert(!RequiresICE && "Can't require vector ICE");
----------------
aaron.ballman wrote:
> 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.
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.


================
Comment at: clang/test/CodeGen/builtin-bswap128.c:1
+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s
+
----------------
This test needs a target to be specified; it will fail if run on a target that doesn't support `int128`.


================
Comment at: clang/test/CodeGen/builtin-bswap128.c:7-8
+// CHECK-NEXT:    [[R2:%.*]] = alloca i128, align 16
+// CHECK-NEXT:    store i128 1329227995784915872903807060280344576, i128* [[R1:%.*]], align 16
+// CHECK-NEXT:    store i128 2658455991569831745807614120560689152, i128* [[R2:%.*]], align 16
+void test() {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114425