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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:19322-19323
+        RevOp = AArch64ISD::REVW_MERGE_PASSTHRU;
+      Op = LowerToPredicatedOp(DAG.getNode(ISD::BITCAST, DL, NewVT, Op1), DAG,
+                               RevOp);
+      Op = DAG.getNode(ISD::BITCAST, DL, ContainerVT, Op);
----------------
wwei wrote:
> paulwalker-arm wrote:
> > Calling `LowerToPredicatedOp` feels like overkill here compared to `DAG.getNode(RevOp, DL, NewVT, ..., DAG.getUNDEF(NewVT)`
> Since there's no unpredicated `revb/revh/revw` for SVE, `LowerToPredicatedOp` can help to construct a predicate operand, and can handle merge passthru opcode also. If using `DAG.getNode(RevOp, DL, NewVT, ..., DAG.getUNDEF(NewVT)`, we need some extra code to get a predicate operand and pass correct merge passthru operands.
That's a good point, fair enough.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:19332
+  if (MinSVESize == MaxSVESize && MaxSVESize == VT.getSizeInBits() &&
+      ShuffleVectorInst::isReverseMask(ShuffleMask)) {
+    Op = DAG.getNode(ISD::VECTOR_REVERSE, DL, ContainerVT, Op1, Op2);
----------------
Is it valid to use `class Instruction` methods this far into code generation?  That said, `ShuffleVectorInst::isReverseMask` looks potentially unsafe in this context anyway because given a 4 element mask it'll return true for both `3, 2, 1, 0` and `7, 6, 5, 4` but as mentioned below `ISD::VECTOR_REVERSE` only supports a single operand and so you need to know specifically which of these scenarios apply.

Do you care about the `7, 6, 5, 4` case? I can see `test_revv8i32v8i32` is a test for this but I think that is being simplified to a `3, 2, 1, 0` based shuffle before you get here.

I'll note that `ARMISelLowering.cpp` has the `isReverseMask` helper function that could be useful.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:19333
+      ShuffleVectorInst::isReverseMask(ShuffleMask)) {
+    Op = DAG.getNode(ISD::VECTOR_REVERSE, DL, ContainerVT, Op1, Op2);
+    return convertFromScalableVector(DAG, VT, Op);
----------------
This looks wrong because ISD::VECTOR_REVERSE only takes a single operand? I presume `Op2` it just been ignored and there's nothing in getNode to ensure only a single operand.


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-permute-rev.ll:202-203
+
+define void @test_revv8i32(<8 x i32>* %a) #0 {
+; CHECK-LABEL: test_revv8i32:
+; CHECK:       // %bb.0:
----------------
I originally wrote a comment asking why the i32 case emitted worse code than the f32 one but then spotted the attribute difference, so this seems more like a negative test.  It's worth adding a function comment to make this clear to the user, which also goes for the other "negative" tests.  Perhaps it's also worth adding `_vl256` to functions where `vscale_range` is set just to drive the point home?


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-permute-rev.ll:270
+  %tmp3 = shufflevector <8 x float> %tmp1, <8 x float> undef, <8 x i32> <i32 1, i32 0, i32 3, i32 2, i32 5, i32 4, i32 7, i32 6>
+  %tmp4 = fadd <8 x float> %tmp2, %tmp3
+  store <8 x float> %tmp4, <8 x float>* %a
----------------
Is there a need for `fadd` here? I'm kind of assuming that without it the float shuffles are being converted to integer ones and thus you lost test coverage? but then I can see `test_revv8f32` doesn't need it so figured I'd ask.


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

https://reviews.llvm.org/D114960