[PATCH] D82665: [AArch64][SVE] Add bfloat16 to outstanding tuple vector intrinsics

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.


================
Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_create2-bfloat.c:9
+// A simple used,unused... macro, long enough to represent any SVE builtin.
+#define SVE_ACLE_FUNC(A1,A2_UNUSED,A3,A4_UNUSED) A1##A3
+#else
----------------
fpetrogalli wrote:
> Nit: these are all new files, so you can safely use clang-format on them, as they will not modify the formatting of pre-existing tests. I know this is not what we have done for most of the files in the ACLE tests, but it makes life so much easier to just run a `git clang-format HEAD^` on a patch and kinda forgetting about formatting! Since they are new files, they will not generate conflicts with anything we might have downstream. FWIW, this is my personal preference, so if you prefer to adhere with the manual formatting of other files, I am happy with it.
I agree it's nice to be able to use clang-format and not think about it but in my opinion it's more important to be consistent in formatting across the ACLE tests. I'd prefer a follow-up patch once the ACLE is complete containing the diff produced from running clang-format on `clang/test/CodeGen/aarch64-sve-intrinsics` / `clang/test/CodeGen/aarch64-sve2-intrinsics`.


================
Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_get2-bfloat.c:20
+  // expected-warning at +1 {{implicit declaration of function 'svget2_bf16'}}
+  return SVE_ACLE_FUNC(svget2,_bf16,,)(tuple, 0);
+}
----------------
fpetrogalli wrote:
> Shouldn't we test also values other then zero? 0,1 for get2, 0,1,2 for get3... 
> Shouldn't we test also values other then zero? 0,1 for get2, 0,1,2 for get3...

We have coverage for all valid indexes in the test suite for these intrinsics, I was about to say it's the same code path regardless of type, which it is on the LLVM side but the intrinsic defs in clang are split so I guess that isn't covered. I'll add extra tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82665