[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);
----------------
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.)


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:2628-2629
+
+  Value *Shuffle = Builder.CreateShuffleVector(NewBOI, SVI.getShuffleMask());
+  return replaceInstUsesWith(SVI, Shuffle);
+}
----------------
I'd prefer to use the raw creator for the final instruction:
  return new ShuffleVectorInst(NewBOI, SVI.getShuffleMask());

This gives a cosmetic/continuity improvement to the text output because InstCombine will transfer the name of the existing value to the new value if we use this code pattern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135876