[PATCH] D147373: [InstCombine] fold double reverses

[Zhengyang Liu via Phabricator via llvm-commits]

liuz marked 2 inline comments as done.
liuz added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:357
+  if (!C->getSplatValue())
+    return false;
+
----------------
liuz wrote:
> RKSimon wrote:
> > Why does this need to be a splat? If its constant we can just constant fold with the outer shuffle
> Thanks Simon for the comment, yes this does not need to be a splat, let me update this code to handle any constants.
Updated the patch to support general form  `(reverse (cmp (reverse x), y)) -> (cmp x, (reverse y))`  and  `(reverse (cmp y, (reverse x))) -> (cmp (reverse y), x)`. This is exactly what Simon is suggesting in https://github.com/llvm/llvm-project/issues/48960#issuecomment-981040032 , except that this one takes cmpinst instead of binops, see the updated test cases. I'll add binops and other cases once this patch is approved.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:361
+    return false;
+  ShuffleVectorInst *InnerRev = cast<ShuffleVectorInst>(InnerIst);
+  if (!InnerRev->isReverse())
----------------
xbolva00 wrote:
> dyn_cast without isa check?
fixed. thanks.


================
Comment at: llvm/test/Transforms/VectorCombine/reverse-loop.ll:141
+  ret <8 x i1> %t3
+}
----------------
RKSimon wrote:
> Add negative test coverage that checks for length changing shuffles?
added.


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

https://reviews.llvm.org/D147373