[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:383
+  if (OutputMappings.find(Input) != OutputMappings.end())
+    return OutputMappings.find(Input)->second;
+  return Input;
----------------
This is being computed twice?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1356
+  bool Inserted = false;
+  PHIBlock = BasicBlock::Create(ReturnBB->getContext(), "phi_block",
+                                ReturnBB->getParent());
----------------



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1361
+
+  // We find the predecessors in the overall function here.
+  SmallVector<BranchInst *, 2> BranchesToChange;
----------------
This comment would be more useful if it said something like "find the predecessors in order to (blah)"


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1390
+/// function.
+static Value *getPassedArgument(Argument *A, OutlinableRegion &Region,
+                                bool ReplacedWithOutlinedCall) {
----------------
I think I'd prefer two functions here rather than one with a flag? It'd be clearer what's going on.

Something like

- `getPassedArgumentAndAdjustArgumentLocation(Argument *A, OutlinableRegion &Region)`
- `getPassedArgumentInAlreadyOutlinedFunction(Argument *A, const OutlinableRegion &Region)`

Also I think `A` can probably be `const`?


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

https://reviews.llvm.org/D106995