[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:2624-2625
+      dyn_cast<Instruction>(Builder.CreateBinOp(BinOp->getOpcode(), X, Y));
+  if (!NewBOI)
+    return nullptr;
+  NewBOI->copyIRFlags(BinOp);
----------------
MattDevereau wrote:
> spatel wrote:
> > I prefer the form that I suggested (and was copied from nearby code):
> >   if (auto *NewBOI = dyn_cast<Instruction>(NewBO))
> >     NewBOI->copyIRFlags(&I);
> > 
> > Ie, it's better to continue here rather than give up. If we've somehow encountered a constant-folding opportunity, then we'll create the final expected (shuffled) constant right here instead of leaving it to some other transforms. 
> > 
> > (And again, I'm not sure how to write a regression test for this.)
> Ah, sorry, I missed the fact that the flags would still propagate here when called from NewBOI. I'll update this with your suggestion.
> I'm assuming that `NewBOI->copyIRFlags(&I);` here should be `NewBOI->copyIRFlags(NewBO);`? `&I` does not exist in this scope, and if you meant `&SVI` this would not copy the IR flags on the binops.
Sorry - that was a copy/paste leftover. We want to transfer flags from the existing binop, so the current code in the patch is right about that part. 

My suggestion should have been:
  if (auto *NewBOI = dyn_cast<Instruction>(NewBO))
    NewBOI->copyIRFlags(BinOp);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135876