[PATCH] D121311: [IROutliner] Ignore Regions where part of an outlined phi nodes incoming block is included, but the final branch instruction is not

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:266
   // check if there are predecessors outside of the region, if there are,
   // we ignore this region since we are unable to handle the severing of the
   // phi node right now. 
----------------
> since we are unable to handle the severing of the phi node right now.

Should this be a TODO?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:281
+      // case, the branch from this block must also be outlined to be valid.
+      if (PN->getIncomingBlock(i) == EndBB &&
+          BackInst->getParent() == EndBB) {
----------------
Nit: Pull `PN->getIncomingBlock(i)` into a variable?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:282
+      if (PN->getIncomingBlock(i) == EndBB &&
+          BackInst->getParent() == EndBB) {
+        if (EndBB->getTerminator() != BackInst)
----------------
Isn't this always true since `EndBB = BackInst->getParent()`? Could this check be removed?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:283
+          BackInst->getParent() == EndBB) {
+        if (EndBB->getTerminator() != BackInst)
+          ++NumPredsOutsideRegion;
----------------
I think this could be hoisted out of the loops and into a variable?

```
bool EndBBTermAndBackInstDifferent = EndBB->getTerminator() != BackInst;
while (PhiNode ...)
```


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:302
+  if (isa<PHINode>(BackInst))
     if (BackInst != &*std::prev(EndBB->getFirstInsertionPt()))
       return;
----------------
Nit: This can all be one `if` now:

```
if (isa<PHINode>(BackInst) && BackInst != &*std::prev(EndBB->getFirstInsertionPt()))
   return;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121311