[PATCH] D135876: [InstCombine] Remove redundant splats in InstCombineVectorOps

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:2620
+  BinaryOperator *NewBinOp =
+      cast<BinaryOperator>(Builder.CreateBinOp(BinOp->getOpcode(), X, Y));
+  if (isa<FPMathOperator>(BinOp))
----------------
spatel wrote:
> This naked cast<> isn't safe (although I'm not sure how to create a test case for it).
> 
> You probably want to do this instead:
>   Value *NewBO = Builder.CreateBinOp(cast<BinaryOperator>(Op0)->getOpcode(), X, Y);
>   if (auto *NewBOI = dyn_cast<Instruction>(NewBO))
>     NewBOI->copyIRFlags(&I);
> 
> But we should have tests that include FMF, nsw, nuw etc to verify this is happening as expected.
Why is this cast unsafe? I was under the impression casting to a BinaryOperator was ok here due to the earlier match against m_BinOp


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

https://reviews.llvm.org/D135876