[PATCH] D87296: [IRSim][IROutliner] Adding support for consolidating functions with different output arguments.

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:378
+static void
+remapExtractedInputs(SetVector<Value *> &ArgInputs,
+                     const DenseMap<Value *, Value *> &OutputMappings,
----------------
I think `ArgInputs` can also be const.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:759
+    Instruction *I = dyn_cast<Instruction>(InstAsUser);
+    I->setDebugLoc(DebugLoc());
+    LLVM_DEBUG(dbgs() << "Move store for instruction " << *I << " to "
----------------
The `dyn_cast<>` followed by dereferencing w/o checking smells fishy. This should either be a `cast<>`, or have an explicit test.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:838
+      if (isa<DbgInfoIntrinsic>(Inst))
+        continue;
+
----------------
What if `FIt` has debug/lifetime instructions that `BB` doesn't? The dual iteration thing going on here seems... iffy.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:862-867
+      if (BBIt != FIt->end())
+        while (BBIt->isLifetimeStartOrEnd() || isa<DbgInfoIntrinsic>(*BBIt)) {
+          BBIt++;
+          if (BBIt == FIt->end())
+            break;
+        }
----------------



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:983
+    BasicBlock *NewBB = BasicBlock::Create(
+        M.getContext(), "output_block_" + std::to_string(Idx),
+        CurrentGroup.OutlinedFunction);
----------------
I think this is a good place to use a Twine, which avoids some unnecessary allocation.


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

https://reviews.llvm.org/D87296