[PATCH] D106995: [IROutliner] Allowing PHINodes in Exit Blocks

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:867
 
+/// We check if the \p V has any uses outside of the region other than \p PN.
+/// If it does, we return true. If it does not, we return false.
----------------
we can be even more concise here:

```
\returns true if \p V has any uses outside its region other than \p PN.
```


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:876
+/// \param BlocksInRegion [in] - The basic blocks contained in the region.
+/// \return whether there are non phi node uses for this value outside of the
+/// region. 
----------------
this is redundant, since you mention it in the first part of the comment.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:884
+  // to keep it as an output.
+  std::vector<unsigned> IncomingNumbers(PN.getNumIncomingValues());
+  std::iota(IncomingNumbers.begin(), IncomingNumbers.end(), 0);
----------------
Would it make sense to use a `SmallVector` here?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:887
+  if (any_of(IncomingNumbers, [PHILoc, &PN, V, &BlocksInRegion](unsigned Idx) {
+        if (Idx == PHILoc)
+          return false;
----------------
couple probably be a one-liner?
```
return (Idx != PhiLoc || V == PN.getIncomingValue(Idx) || !BlocksInRegion.contains(PN.getIncomingBlock(Idx)));
```


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:898
+  // Check if the value is used by any other instructions outside the region.
+  if (any_of(V->users(), [&Exits, &BlocksInRegion](User *U) {
+    Instruction *I = dyn_cast<Instruction>(U);
----------------
Return the `any_of` rather than using an `if`?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:931
+/// Test whether \p ExitBlock contains any PhiNodes that should be considered
+/// outputs. PhiNodes should be outputs when it has an incoming value that
+/// the CodeExtractor marked as an output.
----------------
"it" is unclear IMO

also update comment for updated variable names?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1374
+  SmallVector<BranchInst *, 2> BranchesToChange;
+  for (BasicBlock *Pred : predecessors(ReturnBB)) {
+    BranchesToChange.push_back(cast<BranchInst>(Pred->getTerminator()));
----------------
braces


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1377
+  }
+  for (BranchInst *BI : BranchesToChange) {
+    for (unsigned Succ = 0, End = BI->getNumSuccessors(); Succ < End; Succ++) {
----------------
comment on the successors?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1479
+  // functions.
+  findCanonNumsForPHI(&PN, Region, OutputMappings, PNCanonNums, false);
+
----------------



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

https://reviews.llvm.org/D106995