[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:105
 
+  unsigned PHINodeGVNTracker = -3;
+
----------------
Why this number?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:386
+/// Check the \p OutputMappings structure for value \p Input, if it exists
+/// it has been used as an output for outlining, and has bene renamed, and we
+/// return the new value, otherwise, we return the same value.
----------------



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:880
 
+/// We check if the \p V has any uses outside of the region other than \p PN,
+/// and if it does, we return true, and if it does not, then we return false.
----------------
run on?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:911
+
+  if (!NotOnlyUsedInExitBlocks)
+    return true;
----------------
avoid double negative with variable name?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:921
+    
+    // If the use of the item is inside the region, we skip it since use
+    // inside the region doesn't tell us anything about the uses outside
----------------
this comment is a bit of a run-on


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:946
+
+  return false;
+}
----------------
if you rename `NotOnlyUsedInExitBlocks` to `OnlyUsedInExitBlocks` and change the code accordingly, you can just return that variable.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:971
+    SmallVector<unsigned, 2> IncomingVals;
+    for (unsigned i = 0; i < PN.getNumIncomingValues(); ++i)
+      if (BBSet.contains(PN.getIncomingBlock(i)))
----------------
in for loops like this, it's more idiomatic to write

```
for (unsigned I = 0, E = PN.getNumIncomingValues(); I < E; ++I)
   ...
```


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:978
+    // output.
+    if (IncomingVals.size() < 1)
+      continue;
----------------
Using `empty()` would be more explicit here.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:980
+      continue;
+    else if (IncomingVals.size() == 1) {
+      Value *V = PN.getIncomingValue(*IncomingVals.begin());
----------------
Don't need else since you would have continued already


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1006
+
+using ArgLocWithBBCanon = std::pair<unsigned, unsigned>;
+using CanonList = SmallVector<unsigned, 2>;
----------------
Add comments for what these types represent?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1043
+  CanonList PHIGVNs;
+  for (unsigned Idx = 0; Idx < PN->getNumIncomingValues(); ++Idx) {
+    Value *V = PN->getIncomingValue(Idx);
----------------
Can this use

```
if (any_of(...))
  return None;
```

?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1056
+  // Collect the canonical numbers of the values in the PHINode.
+  for (unsigned Idx = 0; Idx < PN->getNumIncomingValues(); ++Idx) {
+    Value *V = PN->getIncomingValue(Idx);
----------------
Maybe pull `PN->getNumIncomingValues` into a variable to avoid recalculating it across the two loops?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1060
+    GVN = *C.getCanonicalNum(GVN);
+    PHIGVNs.push_back(GVN);
+  }
----------------
actually, can this loop be merged into the previous one?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1066
+  Optional<unsigned> BBGVN = C.getGVN(PHIBB);
+  BBGVN = C.getCanonicalNum(BBGVN.getValue());
+  // Create a pair of the exit block canonical value, and the aggregate
----------------
what happens if `BBGVN` doesn't have a value?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1412
+      unsigned ArgNum = A->getArgNo();
+      if (AdjustArgNo && Region.AggArgToConstant.count(ArgNum) == 0) {
+        ArgNum = Region.AggArgToExtracted.find(ArgNum)->second;
----------------
comments?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1462
+    bool GVNMatches = true;
+    for (unsigned CanonNum : PNCanonNums) {
+      if (CurrentCanonNums.contains(CanonNum))
----------------
this looks like it could be

```
bool GVNMatches = all_of(...);
```

or even better:

```
if (all_of(...))
   return CurrPN; 
```


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:2188
       assert(OV.hasValue() && "Could not find value for GVN?");
-      Value *V = OV.getValue();
+      Value *V = V = OV.getValue();
       InstructionCost LoadCost =
----------------



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:2254
+      Optional<unsigned> OGVN = Candidate.fromCanonicalNum(OutputCanon);
+      Optional<Value *> OV = Candidate.fromGVN(*OGVN);
+
----------------
What happens if `OVGN` doesn't have a value?


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

https://reviews.llvm.org/D106995