[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:110
+  /// -2 and -1 are assigned by the DenseMap.
+  unsigned PHINodeGVNTracker = -3;
+
----------------
Can we assert somewhere that -2 and -1 are actually used by DenseMap?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:884
+  // to keep it as an output.
+  for (unsigned Idx = 0; Idx < PN.getNumIncomingValues(); Idx++) {
+    if (Idx == PHILoc)
----------------
avoid recalculating loop invariant


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:889
+    Value *IncomingValue = PN.getIncomingValue(Idx);
+    if (V != IncomingValue)
+      continue;
----------------
probably don't need the variable here


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:891
+      continue;
+    if (BBSet.contains(IncomingBlock))
+      continue;
----------------
probably don't need the variable here either


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:918
+    // caught when we analyze that PHINode.
+    if (!Exits.contains(Parent)) {
+      return true;
----------------
drop braces


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:955
+    // output.
+    if (IncomingVals.empty())
+      continue;
----------------
we calculate `IncomingVals.size()s` twice here

how about

```
unsigned NumIncoming = IncomingVals.size();

if (NumIncoming == 0)
  continue;

if (NumIncoming == 1) {
   ...
}
```


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:958
+    
+    if (IncomingVals.size() == 1) {
+      Value *V = PN.getIncomingValue(*IncomingVals.begin());
----------------
comments here?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1007
+/// the region.  We create a hash code based on the Canonical number of the
+/// parent BasicBlock, the cnaonical numbering of the values stored in the
+/// PHINode and the aggregate argument location.  This is used to find whether
----------------



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1185
   }
+  stable_sort(Region.GVNStores);
 }
----------------
comment?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1406
+      if (AdjustArgNo && Region.AggArgToConstant.count(ArgNum) == 0) {
+        ArgNum = Region.AggArgToExtracted.find(ArgNum)->second;
+        IVal = CI->getArgOperand(ArgNum);
----------------
can you do this like

```
if (!AdjustArgNo) {
// The call instruction was already removed when we created a call to an outlined function. So, we have all the arguments already. Just grab the argument.
IVal = CI->getArgOperand(ArgNum);
continue;
}

if (Region.AggArgToConstant.count(ArgNum)) {
  // The argument is/isn't a constant...
  // do stuff
  continue;
}

// Other case

```


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1452
+  // the uses of the PHINode we are searching for, with the found PHINode.
+  for (Instruction &Inst : *OverallPhiBlock) {
+    PHINode *CurrPN = dyn_cast<PHINode>(&Inst);
----------------
use `phis()`?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1469
+  NewPN->insertBefore(&*OverallPhiBlock->begin());
+  for (unsigned Idx = 0; Idx < NewPN->getNumIncomingValues(); Idx++) {
+    Value *IVal = NewPN->getIncomingValue(Idx);
----------------
avoid recalculating loop invariant


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1475
+    // block.
+    Instruction *Inst = IBlock->getFirstNonPHI();
+    Value * TempVal = Region.findCorrespondingValueIn(*First, Inst);
----------------
`getFirstNonPHI` can return nullptr. assert?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:2200
+    for (unsigned OutputCanon : Region->GVNStores) {
+      if (OutputCanon > CurrentGroup.PHINodeGVNTracker) {
+        auto It = CurrentGroup.PHINodeGVNToGVNs.find(OutputCanon);
----------------
comment?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:2210
+          Region->Candidate->fromCanonicalNum(OutputCanon);
+      Optional<Value *> OV = Region->Candidate->fromGVN(*OGVN);
       assert(OV.hasValue() && "Could not find value for GVN?");
----------------
why is there an assert below and not here?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:2267
+    for (unsigned OutputCanon : OutputUse) {
+      if (OutputCanon > CurrentGroup.PHINodeGVNTracker) {
+        auto It = CurrentGroup.PHINodeGVNToGVNs.find(OutputCanon);
----------------
code duplication with change above?


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

https://reviews.llvm.org/D106995