[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

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

327

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

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

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/include/llvm/CodeGen/ValueTypes.h

public:
     EVT getVectorElementType() const {
       assert(isVector() && "Invalid vector type!");
       if (isSimple())
         return V.getVectorElementType();
       return getExtendedVectorElementType();
     }
 
     /// Given a vector type, return the number of elements it contains.
+    /// This assumes a fixed-vector type
     unsigned getVectorNumElements() const {
 #ifdef STRICT_FIXED_SIZE_VECTORS
       assert(isFixedLengthVector() && "Invalid vector type!");
 #else
       assert(isVector() && "Invalid vector type!");
       if (isScalableVector())
         WithColor::warning()
             << "Possible incorrect use of EVT::getVectorNumElements() for "
#endif
     ElementCount getVectorElementCount() const {
       assert((isVector()) && "Invalid vector type!");
       if (isSimple())
         return V.getVectorElementCount();
 
       return getExtendedVectorElementCount();
     }
 
-    /// 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.
     unsigned getVectorMinNumElements() const {
       return getVectorElementCount().getKnownMinValue();
     }
 
     /// Return the size of the specified value type in bits.
     ///
     /// If the value type is a scalable vector type, the scalable property will
     /// be set and the runtime size will be a positive integer multiple of the

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.getVectorMinNumElements() != 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/vselect-sve-interface-warnings.ll

+; RUN: llc -mtriple=aarch64-linux-unknown -mattr=+sve -o /dev/null < %s 2>&1 | FileCheck %s  --implicit-check-not "warning" --allow-empty
+
+define <vscale x 8 x i8> @vselect_cmp_ne(<vscale x 8 x i8> %a, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c) {
+  %cmp = icmp ne <vscale x 8 x i8> %a, %b
+  %d = select <vscale x 8 x i1> %cmp, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c
+  ret <vscale x 8 x i8> %d
+}
+
+define <vscale x 8 x i8> @vselect_cmp_eq(<vscale x 8 x i8> %a, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c) {
+  %cmp = icmp eq <vscale x 8 x i8> %a, %b
+  %d = select <vscale x 8 x i1> %cmp, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c
+  ret <vscale x 8 x i8> %d
+}
+
+define <vscale x 8 x i8> @vselect_cmp_sgt(<vscale x 8 x i8> %a, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c) {
+  %cmp = icmp sgt <vscale x 8 x i8> %a, %b
+  %d = select <vscale x 8 x i1> %cmp, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c
+  ret <vscale x 8 x i8> %d
+}
+
+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
+  %d = select <vscale x 8 x i1> %cmp, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c
+  ret <vscale x 8 x i8> %d
+}
+
+define <vscale x 8 x i8> @vselect_cmp_slt(<vscale x 8 x i8> %a, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c) {
+  %cmp = icmp slt <vscale x 8 x i8> %a, %b
+  %d = select <vscale x 8 x i1> %cmp, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c
+  ret <vscale x 8 x i8> %d
+}
+
+define <vscale x 8 x i8> @vselect_cmp_sle(<vscale x 8 x i8> %a, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c) {
+  %cmp = icmp sle <vscale x 8 x i8> %a, %b
+  %d = select <vscale x 8 x i1> %cmp, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c
+  ret <vscale x 8 x i8> %d
+}
+
+define <vscale x 8 x i8> @vselect_cmp_ugt(<vscale x 8 x i8> %a, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c) {
+  %cmp = icmp ugt <vscale x 8 x i8> %a, %b
+  %d = select <vscale x 8 x i1> %cmp, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c
+  ret <vscale x 8 x i8> %d
+}
+
+define <vscale x 8 x i8> @vselect_cmp_uge(<vscale x 8 x i8> %a, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c) {
+  %cmp = icmp uge <vscale x 8 x i8> %a, %b
+  %d = select <vscale x 8 x i1> %cmp, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c
+  ret <vscale x 8 x i8> %d
+}
+
+define <vscale x 8 x i8> @vselect_cmp_ult(<vscale x 8 x i8> %a, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c) {
+  %cmp = icmp ult <vscale x 8 x i8> %a, %b
+  %d = select <vscale x 8 x i1> %cmp, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c
+  ret <vscale x 8 x i8> %d
+}
+
+define <vscale x 8 x i8> @vselect_cmp_ule(<vscale x 8 x i8> %a, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c) {
+  %cmp = icmp ule <vscale x 8 x i8> %a, %b
+  %d = select <vscale x 8 x i1> %cmp, <vscale x 8 x i8> %b, <vscale x 8 x i8> %c
+  ret <vscale x 8 x i8> %d
+}