[PATCH] D98249: [SVE] Suppress vselect warning from incorrect interface call

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Hi @nasherm, the fix looks good - just a few minor comments!



================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:301
     /// Given a vector type, return the number of elements it contains.
+    /// This assumes a fixed-vector type
     unsigned getVectorNumElements() const {
----------------
nit: I think this needs a full stop at the end.


================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:327
 
-    /// Given a vector type, return the minimum number of elements it contains.
+    /// Given a (possibly scalable) vector type,
+    /// return the minimum number of elements it contains.
----------------
nit: It would be good to use up to 80 chars on the first line of the comment in line with the other comments.


================
Comment at: llvm/test/CodeGen/AArch64/vselect-sve-interface-warnings.ll:1
+; RUN: llc -mtriple=aarch64-linux-unknown -mattr=+sve -o /dev/null < %s 2>&1 | FileCheck %s  --implicit-check-not "warning" --allow-empty
+
----------------
The way we've done in this other tests is to combine the codegen with the warnings check, i.e.

  ; RUN: llc -mtriple=aarch64--linux-gnu -mattr=+sve < %s 2>%t | FileCheck %s
  ; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t

whereas here you're only checking for warnings. I think it would be good to check the generated code here too in a similar way and a comment to the test describing how the user can fix any warnings that may fire in this test. Again, we've added this to other tests:

  ; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.

Also, the naming convention we've used for other SVE tests is usually something like sve-fcmp.ll. It would be good if you could something similar, e.g. sve-vselect.ll


================
Comment at: llvm/test/CodeGen/AArch64/vselect-sve-interface-warnings.ll:21
+
+define <vscale x 8 x i8> @vselect_cmp_sge(<vscale x 8 x i8> %a, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c) {
+  %cmp = icmp sge <vscale x 8 x i8> %a, %b
----------------
>From the code you've fixed it looks like we should go down the same path for all the comparison opcodes so maybe you only need to test one or two cases, e.g. "icmp ne", "icmp sgt", or something like that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98249