[PATCH] D89911: [PartialInliner]: Handle code regions in a switch stmt cases

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/PartialInlining.cpp:507
       // We can only outline single entry regions (for now).
-      if (!IsSingleEntry(DominateVector))
+      if (!IsSingleEntry(DominateVector)) {
+        LLVM_DEBUG(dbgs() << "Block " << SI->getName()
----------------
It seems like it would be simpler to get rid of the isSingleEntry lambda all together and inline the checks.

The check ` BlockList.size() >= 1` in particular seems redundant. The only case where `DominateVector` may be empty is if unreachable I think. `SI` is a successor of a node we found during DFS traversal from the entry block, so all `SI`s should be reachable? So could this just be something like

```
  assert(!DominateVector.empty() && "SI should be reachable and have at least itself as descendant");
  // We can only outline single entry regions (for now).
  if (!DominateVector.front()->hasNPredecessors(1)) {
...
```


================
Comment at: llvm/lib/Transforms/IPO/PartialInlining.cpp:509
+        LLVM_DEBUG(dbgs() << "Block " << SI->getName()
+                          << " doesn't have a single predecessor\n";);
         continue;
----------------

nit: isSingleEntry checks the predecessors in the dom tree, not the direct predecessors of the node. Might be good to clarify


================
Comment at: llvm/lib/Transforms/IPO/PartialInlining.cpp:537
+
+        LLVM_DEBUG(dbgs() << "ABORT: Outline region cost is smaller than "
+                          << MinOutlineRegionCost << "\n";);
----------------
I am not sure why we say `ABORT: ` here but not at the other, similar continues?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89911