[PATCH] D66965: [InstCombine] recognize bswap disguised as shufflevector

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Some more thoughts, but i think this looks ok.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:2424-2426
+    if (Shuf->hasOneUse() && Shuf->isReverse() && (NumShufElts % 2) == 0 &&
+        SrcTy->getScalarSizeInBits() == 8 && DestTy->isIntegerTy() &&
+        DL.isLegalInteger(DestTy->getScalarSizeInBits())) {
----------------
Should `Shuf->isReverse()` be checked last, it is likely most costly check here?


================
Comment at: llvm/test/Transforms/InstCombine/bswap.ll:284
 }
 
+; Negative test - scalar type is not in the data layout
----------------
I think you also want a test where the shuffle mask is widening (to a legal type)
We currently won't accept it, but we might one day, and i'm not sure what that code would do.


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

https://reviews.llvm.org/D66965