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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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))
----------------
MattDevereau wrote:
> 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
To be clear, this cast is ok because we matched Op0 with m_BinOp:
  auto BinOp = cast<BinaryOperator>(Op0); 
(but prefer `auto *` according to the coding standards)

This cast is not because the Builder can fold its result to a constant Value:
      cast<BinaryOperator>(Builder.CreateBinOp(BinOp->getOpcode(), X, Y));


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

https://reviews.llvm.org/D135876