[SVE][VLS] Don't combine logical AND.

[fpetrogalli]

Summary

This patch makes sure that fixed length SVE code generation does not fail trying to generate the NEON bic instruction when operating with vector wider that 128 bits. For vector bigger than 128-bit, the SVE and instruction is used.

The behavior on NEON vectors is preserved and guarded with the two 128-bit and 64-bit tests.

[paulwalker-arm]

I do not understand the new tests. At first glance they look identical to those in sve-fixed-length-int-log.ll and when I test them against HEAD they pass as is (i.e. without the fix to performANDCombine). I suspect you need a more complex test to exercise the buggy code path.

Whilst discussing the testing I'd prefer to keep the SVE fixed length tests under the sve-fixed-length-* namespace and given the change I doubt you need test files for multiple variants of SVE. If if works for SVE256 then why would it break for larger implementations. You could follow a similar style as sve-fixed-length-subvector.ll or sve-fixed-length-int-log.ll (note the GE LE difference).

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

11162

Does the combine also apply to 64-bit vectors? I know the code it correct but the comment might need updating. If it helps I normally use "NEON sized vectors" or "NEON vectors" to cover all the cases.

[fpetrogalli]

I have updated the tests to make sure they are related to the code that has been modified.

[paulwalker-arm]

A couple of recommendations to consider before landing the patch.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

11160

Should people know what VLS means? Personally I think you can drop the reference since "It does not work for SVE when dealing with vectors larger than 128bits." should be clear enough.

11162

At some point we might have a genuine larger than NEON combine here at which point we should protect the NEON specific code with .is64BitVector || is128BitVector, but I guess right now this'll do.

llvm/test/CodeGen/AArch64/vls-sve-512-and.ll

1 ↗

(On Diff #283934)

I'm still going to recommend again that you create tests using the sve-fixed-length- prefix so all relevant testing is kept together.

[fpetrogalli]

Thank you for the review @paulwalker-arm.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

static SDValue performANDCombine(SDNode *N,
   SDValue LHS = N->getOperand(0);
   EVT VT = N->getValueType(0);
   if (!VT.isVector() || !DAG.getTargetLoweringInfo().isTypeLegal(VT))
     return SDValue();
 
   if (VT.isScalableVector())
     return performSVEAndCombine(N, DCI);
 
+  // The combining code below works only for NEON vectors. In particular, it
+  // does not work for SVE when dealing with vectors wider than 128 bits.
+  if (!(VT.is64BitVector() || VT.is128BitVector()))
+    return SDValue();
+
   BuildVectorSDNode *BVN =
       dyn_cast<BuildVectorSDNode>(N->getOperand(1).getNode());
   if (!BVN)
     return SDValue();
 
   // AND does not accept an immediate, so check if we can use a BIC immediate
   // instruction instead. We do this here instead of using a (and x, (mvni imm))
   // pattern in isel, because some immediates may be lowered to the preferred

llvm/test/CodeGen/AArch64/sve-fix-length-and-combine-512.ll

+; RUN: llc -mtriple=aarch64-linux-gnu -mattr=+sve -aarch64-sve-vector-bits-min=512  -o - -asm-verbose=0 < %s | FileCheck %s
+
+; CHECK-LABEL: vls_sve_and_64xi8:
+; CHECK-NEXT:  adrp    x[[ONE:[0-9]+]], .LCPI0_0
+; CHECK-NEXT:  ptrue   p0.b, vl64
+; CHECK-NEXT:  add     x[[TWO:[0-9]+]], x[[ONE]], :lo12:.LCPI0_0
+; CHECK-NEXT:  ld1b    { z0.b }, p0/z, [x0]
+; CHECK-NEXT:  ld1b    { z1.b }, p0/z, [x[[TWO]]]
+; CHECK-NEXT:  and     z0.d, z0.d, z1.d
+; CHECK-NEXT:  st1b    { z0.b }, p0, [x1]
+; CHECK-NEXT:  ret
+define void @vls_sve_and_64xi8(<64 x i8>* %ap, <64 x i8>* %out) nounwind {
+ %a = load <64 x i8>, <64 x i8>* %ap
+ %b = and <64 x i8> %a, <i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255,
+                         i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255,
+                         i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255,
+                         i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255>
+ store <64 x i8> %b, <64 x i8>* %out
+ ret void
+}
+
+; CHECK-LABEL: vls_sve_and_16xi8:
+; CHECK-NEXT:  bic     v0.8h, #255
+; CHECK-NEXT:  ret
+define <16 x i8> @vls_sve_and_16xi8(<16 x i8> %b, <16 x i8>* %out) nounwind {
+ %c = and <16 x i8> %b, <i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255>
+ ret <16 x i8> %c
+}
+
+; CHECK-LABEL: vls_sve_and_8xi8:
+; CHECK-NEXT:  bic     v0.4h, #255
+; CHECK-NEXT:  ret
+define <8 x i8> @vls_sve_and_8xi8(<8 x i8> %b, <8 x i8>* %out) nounwind {
+ %c = and <8 x i8> %b, <i8 0, i8 255, i8 0, i8 255, i8 0, i8 255, i8 0, i8 255>
+ ret <8 x i8> %c
+}
+