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

weiwei via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 10 04:25:51 PST 2021


wwei added inline comments.


================
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);
----------------
wwei wrote:
> paulwalker-arm wrote:
> > 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.
> Maybe we use `ShuffleVectorInst::isReverseMask` here will be better, because it will call `isSingleSourceMask` to ensure the shuffle mask chooses elements from exactly one source vector(`isReverseMask` in `ARMISelLowering.cpp` will not check this). 
> I tried various test case forms, but I couldn’t construct this kind(`7, 6, 5, 4`) of case. It would always be converted to `3, 2, 1, 0` form. To be on the safe side, I still added a check-- `Op2.isUndef()`, and `Op1` will be the only valid operand for `VECTOR_REVERSE`
And for `REVB/REVH/REVW` cases, I added `Op2.isUndef()` check too.


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

https://reviews.llvm.org/D114960



More information about the llvm-commits mailing list