[SVE] Suppress vselect warning from incorrect interface call

[nasherm]

Summary

The VSelectCombine handler within AArch64ISelLowering,
uses an interface call which only expects fixed vectors.
This generates a warning when the call is made on a
scalable vector. This warning has been suppressed with this change,
by using the ElementCount interface, which supports both fixed and scalable vectors.
I have also added a regression test which recreates the warning.

[david-arm]

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

llvm/include/llvm/CodeGen/ValueTypes.h

301 ↗

(On Diff #329291)

nit: I think this needs a full stop at the end.

327 ↗

(On Diff #329291)

nit: It would be good to use up to 80 chars on the first line of the comment in line with the other comments.

llvm/test/CodeGen/AArch64/vselect-sve-interface-warnings.ll

1 ↗

(On Diff #329291)

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

21 ↗

(On Diff #329291)

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?

[nasherm]

Response to David's comments

[nasherm]

Slight change to the checking call which triggers the warning. We now
check using the ElementCount interface.

[sdesmalen]

Just two more nits before I'm happy to accept.

llvm/test/CodeGen/AArch64/sve-vselect-interface-warnings.ll

1 ↗

(On Diff #331204)

Can you rename the file to sve-cmp-select.ll ?

We're planning to remove the 'warning' mechanism in the future, so it's better not to have warning in the name of files.

7 ↗

(On Diff #331204)

Please change the types to <vscale x 16 x i8> (and <vscale x 16 x i1> for the predicate in the select), as that simplifies the resulting code a bit (i.e. no type legalization is necessary then).

[nasherm]

Addressed Sander's comments regarding renaming and using <vselect x 16 x ty> types.

[nasherm]

Removed changes to comments within ValueType.h

[sdesmalen]

LGTM!

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

if (CmpLHS.getValueType() == N->getOperand(1).getValueType() &&
       SDValue Val = DAG.getBuildVector(VT, SDLoc(N), Ops);
 
       auto Shift = DAG.getNode(ISD::SRA, SDLoc(N), VT, CmpLHS, Val);
       auto Or = DAG.getNode(ISD::OR, SDLoc(N), VT, Shift, N->getOperand(1));
       return Or;
     }
   }
 
-  if (N0.getOpcode() != ISD::SETCC || CCVT.getVectorNumElements() != 1 ||
+  if (N0.getOpcode() != ISD::SETCC ||
+      CCVT.getVectorElementCount() != ElementCount::getFixed(1) ||
       CCVT.getVectorElementType() != MVT::i1)
     return SDValue();
 
   EVT ResVT = N->getValueType(0);
   EVT CmpVT = N0.getOperand(0).getValueType();
   // Only combine when the result type is of the same size as the compared
   // operands.
   if (ResVT.getSizeInBits() != CmpVT.getSizeInBits())

llvm/test/CodeGen/AArch64/sve-cmp-select.ll

+; RUN: llc -mtriple=aarch64-linux-unknown -mattr=+sve -o - < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix="WARN" --allow-empty %s <%t
+
+; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.
+; WARN-NOT: warning
+
+define <vscale x 16 x i8> @vselect_cmp_ne(<vscale x 16 x i8> %a, <vscale x 16 x i8> %b, <vscale x 16 x i8> %c) {
+  ; CHECK-LABEL: vselect_cmp_ne
+  ; CHECK:       // %bb.0:
+  ; CHECK-NEXT:    ptrue  p0.b
+  ; CHECK-NEXT:    cmpne  p0.b, p0/z, z0.b, z1.b
+  ; CHECK-NEXT:    sel  z0.b, p0, z1.b, z2.b
+  ; CHECK-NEXT:    ret
+  %cmp = icmp ne <vscale x 16 x i8> %a, %b
+  %d = select <vscale x 16 x i1> %cmp, <vscale x 16 x i8> %b, <vscale x 16 x i8> %c
+  ret <vscale x 16 x i8> %d
+}
+
+define <vscale x 16 x i8> @vselect_cmp_sgt(<vscale x 16 x i8> %a, <vscale x 16 x i8> %b, <vscale x 16 x i8> %c) {
+  ; CHECK-LABEL: vselect_cmp_sgt
+  ; CHECK:       // %bb.0:
+  ; CHECK-NEXT:   ptrue p0.b
+  ; CHECK-NEXT:   cmpgt p0.b, p0/z, z0.b, z1.b
+  ; CHECK-NEXT:   sel z0.b, p0, z1.b, z2.b
+  ; CHECK-NEXT:   ret
+  %cmp = icmp sgt <vscale x 16 x i8> %a, %b
+  %d = select <vscale x 16 x i1> %cmp, <vscale x 16 x i8> %b, <vscale x 16 x i8> %c
+  ret <vscale x 16 x i8> %d
+}
+
+define <vscale x 16 x i8> @vselect_cmp_ugt(<vscale x 16 x i8> %a, <vscale x 16 x i8> %b, <vscale x 16 x i8> %c) {
+  ; CHECK-LABEL: vselect_cmp_ugt
+  ; CHECK:       // %bb.0:
+  ; CHECK-NEXT:   ptrue p0.b
+  ; CHECK-NEXT:   cmphi p0.b, p0/z, z0.b, z1.b
+  ; CHECK-NEXT:   sel z0.b, p0, z1.b, z2.b
+  ; CHECK-NEXT:   ret
+  %cmp = icmp ugt <vscale x 16 x i8> %a, %b
+  %d = select <vscale x 16 x i1> %cmp, <vscale x 16 x i8> %b, <vscale x 16 x i8> %c
+  ret <vscale x 16 x i8> %d
+}