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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
