[PATCH] D48830: [InstCombine] fold shuffle-with-binop and common value

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

I don't have any notable feedback, so other than nits, looks good.
Maybe i'm missing something and you want to wait for a second review[er].



================
Comment at: lib/Transforms/InstCombine/InstCombineVectorOps.cpp:1158
+
+  auto BOpcode = cast<BinaryOperator>(Op0IsBinop ? Op0 : Op1)->getOpcode();
+  // TODO: Allow div/rem by accounting for potential UB due to undef elements.
----------------
Would it be simpler to do 
```
Value *OpV = Op0IsBinop ? Op0 : Op1
Value *OpC = Op0IsBinop ? Op1 : Op0
```
and use these later on?


================
Comment at: lib/Transforms/InstCombine/InstCombineVectorOps.cpp:1166
+  // If there's no identity constant for this binop, we're done.
+  Constant *IdC = ConstantExpr::getBinOpIdentity(BOpcode, Shuf.getType());
+  if (!IdC)
----------------
Note: `ConstantExpr::getBinOpIdentity()` does not handle `div`/`rem`.


================
Comment at: lib/Transforms/InstCombine/InstCombineVectorOps.cpp:1170-1174
+  // Create a constant with identity values in the lanes that return the
+  // original value.
+  if (!Op0IsBinop)
+    std::swap(C, IdC);
+  Constant *NewC = ConstantExpr::getShuffleVector(C, IdC, Shuf.getMask());
----------------
Nit: i would like the comment to show an example here.



https://reviews.llvm.org/D48830