[PATCH] D121310: [IROutliner] IR Outliner can mix up order of incoming values when compressing phi nodes if contain the same values

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1584
   
-  DenseSet<unsigned> PNCanonNums;
+  SmallVector<std::pair<unsigned, BasicBlock *>, 2> PNCanonNums;
   // We have to use the extracted function since we have merged this region into
----------------
- Nit: I don't think we need an initial value for `SmallVector` anymore
- Could use a comment explaining what the variable is? (Probably the same as `findCanonNumsForPHI`, but future readers may not see that.)


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1601
 
-    if (all_of(PNCanonNums, [&CurrentCanonNums](unsigned CanonNum) {
-          return CurrentCanonNums.contains(CanonNum);
-        }))
+    if (PNCanonNums.size() != CurrentCanonNums.size())
+      continue;
----------------
Could use a comment too?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1605
+    bool FoundMatch = true;
+    for (unsigned Idx = 0, Edx = PNCanonNums.size(); Idx < Edx; ++Idx) {
+      std::pair<unsigned, BasicBlock *> ToCompareTo = CurrentCanonNums[Idx];
----------------
Comment on what you're trying to do here?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1637
+    BasicBlock *BlockToUse =
+        Region.findCorrespondingBlockIn(*FirstRegion, IncomingBlock);
     NewPN->setIncomingBlock(Idx, BlockToUse);
----------------
`findCorrespondingBlockIn` mentions the value could be nullptr. Assert?


================
Comment at: llvm/test/Transforms/IROutliner/different-order-phi-merges.ll:4
+
+; Check that differnetly ordered phi nodes are not matched when merged, instead
+; generating two output paths.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121310