[PATCH] D147373: [InstCombine] fold double reverses

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:2731
+    Value *LHS = CI->getOperand(0), *RHS = CI->getOperand(1);
+    CmpInst::Predicate Pred = CI->getPredicate();
+
----------------
Can we inline this into the call to `CmpInst::Create` like we did with CI->getOpcode()?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:2736
+    Value *Y;
+    if (RevX = dyn_cast<ShuffleVectorInst>(LHS),
+        RevX && RevX->hasOneUse() && RevX->isReverse()) {
----------------
You can do

```
if ((RevX = dyn_cast<ShuffleVectorInst>(LHS)) &&
    RevX->hasOneUse() && RevX->isReverse())
```


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:2751
+    Constant *Poisons = PoisonValue::get(RevX->getType());
+    if (Constant *C = dyn_cast<Constant>(Y)) {
+      RevY = ConstantExpr::getShuffleVector(C, Poisons, OuterMask);
----------------
Why do we need to check for Constant here? Builder will constant fold.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:2754
+    } else {
+      RevY = Builder.CreateShuffleVector(Y, Poisons, OuterMask);
+    }
----------------
There's a version of CreateShuffleVector that only takes one Value and creates the Poison vector.

Though I wonder if we should just call Builder.CreateVectorReverse instead of copying the mask.


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

https://reviews.llvm.org/D147373