[PATCH] D113376: [AArch64][SVE] Lower shuffles to permute instructions: zip1/2, uzp1/2, trn1/2

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:19267-19268
+  // like zip/uzp/trn/rev from having some wrong or undefined behavior.
+  if (Subtarget->getMinSVEVectorSizeInBits() ==
+      Subtarget->getMaxSVEVectorSizeInBits()) {
+    if (SDValue PermOp =
----------------
I still don't think this is enough as you've missed my previous `I doubt the logic holds for the case when a vector is legal but not the exact size of the target vector register` comment.  Further discussion is attached to the test zip_v32i8 below...

I'm starting to wonder if it's worth breaking out lowerShuffleToZIP_UZP_TRN as I feel like once support is added for more of the combinations there's not likely to be much reuse.  For example taking the zip1/zip2 case I think supporting zip1 is likely simple and just works for all legal VTs, but zip2 requires the VT to be exactly the size of the target vector (i.e. `VT.getSizeInBits() == Subtarget->getMinSVEVectorSizeInBits() == Subtarget->getMaxSVEVectorSizeInBits()`) or perhaps a different instruction sequence.


================
Comment at: llvm/test/CodeGen/AArch64/sve-fix-length-permute-zip-uzp-trn.ll:24-25
+; VBITS_EQ_512-NEXT:    ld1b { z1.b }, p0/z, [x1]
+; VBITS_EQ_512-NEXT:    zip1 z2.b, z0.b, z1.b
+; VBITS_EQ_512-NEXT:    zip2 z0.b, z0.b, z1.b
+; VBITS_EQ_512-NEXT:    add z0.b, p0/m, z0.b, z2.b
----------------
The output for `VBITS_EQ_256` and `VBITS_EQ_512` is the same, which doesn't make sense considering the target vector length is different.

For the `VBITS_EQ_256` case index `16` is the start of the high half of the target vector and thus zip2 works.  But for the `VBITS_EQ_512` case index `16` is actually the start of the second quarter of the target vector and thus zip2 will do the wrong thing.




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

https://reviews.llvm.org/D113376