[PATCH] D114960: [AArch64][SVE] Lower shuffles to permute instructions: rev/revb/revh/revw

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm accepted this revision.
paulwalker-arm added a comment.
This revision is now accepted and ready to land.

Functionally this looks good but I've added a few stylistic comments below to consider before committing.  I found the tests harder to follow than necessary as it wasn't always clear what the intent of a specific test was, especially those when `rev/b/h/w` instructions are not emitted.  More comments would help and I also think having two separate test files, one for `rev (permute-rev)` and another for `revb/revh/revw (permute-rev-elts)` would be advantageous.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:19311
+  for (unsigned LaneSize : {64U, 32U, 16U}) {
+    if (isREVMask(ShuffleMask, VT, LaneSize) && Op2.isUndef()) {
+      EVT NewVT =
----------------
In this instance I don't believe this is required because `isREVMask` does what you need.  It's only the uses of `ShuffleVectorInst::isReverseMask` that need extra protection, which I see you've covered below.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:19322-19324
+      Op = LowerToPredicatedOp(DAG.getNode(ISD::BITCAST, DL, NewVT, Op1), DAG,
+                               RevOp);
+      Op = DAG.getNode(ISD::BITCAST, DL, ContainerVT, Op);
----------------
Personally I think the following looks better:
```
Op = DAG.getNode(ISD::BITCAST, DL, NewVT, Op1);
Op = LowerToPredicatedOp(Op, DAG, RevOp);
Op = DAG.getNode(ISD::BITCAST, DL, ContainerVT, Op);
```
but then I'm also not a fan of how the function's input parameter `Op` is overwritten.


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-permute-rev.ll:97-100
+define void @test_revv8i32(<8 x i32>* %a) #0 {
+; CHECK-LABEL: test_revv8i32:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    stp x29, x30, [sp, #-16]! // 16-byte Folded Spill
----------------
This test either looks misplaced (belongs with the other `rev` tests towards the bottom of this file?) or is perhaps redundant?


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-permute-rev.ll:222
+
+attributes #0 = { "target-features"="+sve" }
+
----------------
Can this be moved to the bottom with the other attributes?


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-permute-rev.ll:283-285
+; Only support to reverse bytes / halfwords / words within elements
+define void @test_revv4i64(<4 x i64>* %a) #1 {
+; CHECK-LABEL: test_revv4i64:
----------------
This test looks like it belongs in the top half after `test_revhv32i16`? (i.e. so it part of the other element shuffle tests as apposed to this blocks of tests which is testing whole vector reversals).


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

https://reviews.llvm.org/D114960