[PATCH] D86306: [InstCombine] PHI-of-insertvalues -> insertvalue-of-PHI's

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

This seems ok to me, but I haven't looked at insertvalue/extractvalue patterns very much, so you might want to get a 2nd opinion.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineInternal.h:620
   Instruction *FoldPHIArgBinOpIntoPHI(PHINode &PN);
+  Instruction *FoldPHIArgInsertValueInstructionIntoPHI(PHINode &PN);
   Instruction *FoldPHIArgGEPIntoPHI(PHINode &PN);
----------------
I view it is an opportunity to update formatting of related code if we are adding similar but new functionality.
Otherwise, we are going to be stuck with those capital letters forever. :)


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:307
+    auto *I = dyn_cast<InsertValueInst>(PN.getIncomingValue(i));
+    if (!I || !I->hasOneUse() || I->getIndices() != FirstIVI->getIndices())
+      return nullptr;
----------------
We are allowing multi-index aggregates, right? If so, we should have at least 1 test to verify that pattern is working as expected.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:313
+  std::array<PHINode *, 2> NewOperands;
+  for (int OpIdx : seq(0U, 2U)) {
+    auto *&NewOperand = NewOperands[OpIdx];
----------------
Testing my limits of C++... is seq() better/different than just listing the values as `{0, 1}`?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:315
+    auto *&NewOperand = NewOperands[OpIdx];
+    // Create a new PHI node to recieve the values the operand has in each
+    // incoming basic block.
----------------
typo: receive


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86306