[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:354
                             SetVector<Value *> &CurrentInputs,
+                            DenseMap<Value *, Value *> &OutputMappings,
                             std::vector<unsigned> &EndInputNumbers) {
----------------
const DenseMap &?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:378
+static void remapExtractedInputs(SetVector<Value *> &ArgInputs,
+                                 DenseMap<Value *, Value *> &OutputMappings,
+                                 SetVector<Value *> &RemappedArgInputs) {
----------------
const DenseMap &?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:404
 /// \param [out] ArgInputs - The values of the inputs to the extracted function.
-static void getCodeExtractorArguments(OutlinableRegion &Region,
-                                      std::vector<unsigned> &InputGVNs,
-                                      DenseSet<unsigned> &NotSame,
-                                      SetVector<Value *> &ArgInputs) {
+/// \param [out] Outputs - The set of values extractored by the CodeExtractor
+/// as outputs.
----------------
extracted


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:537
+findExtractedOutputToOverallOutputMapping(OutlinableRegion &Region,
+                                          SetVector<Value *> Outputs) {
+  OutlinableGroup &Group = *Region.Parent;
----------------
This can also be `ArrayRef`d.  See `SetVector::getArrayRef()`


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:810
+findDuplicateOutputBlock(BasicBlock *OutputBB,
+                         std::vector<BasicBlock *> &OutputStoreBBs) {
+
----------------
Should this be an `ArrayRef`, since the vector contents aren't modified by this function?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:859
+                                 BasicBlock *OutputBB, BasicBlock *EndBB,
+                                 DenseMap<Value *, Value *> &OutputMappings,
+                                 std::vector<BasicBlock *> &OutputStoreBBs) {
----------------
const DenseMap &?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:873
+    auto BBIt = FIt->instructionsWithoutDebug().begin();
+    for (Instruction &Inst : BB.instructionsWithoutDebug()) {
+      if (Inst.isLifetimeStartOrEnd())
----------------
Since you have to fix up for the lifetime intrinsics anyway and there's no instructionsWithoutDebugOrLifetime(), maybe it makes more sense to just iterate over everything, and add the `if (isa<DebugInfoIntrinsic>(Inst)) continue;` check.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:891
+
+      BBIt++;
+      if (BBIt != FIt->instructionsWithoutDebug().end())
----------------
... which makes this part less awkward.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:913
+void createSwitchStatement(Module &M, OutlinableGroup &OG, BasicBlock *EndBB,
+                           std::vector<BasicBlock *> &OutputStoreBBs) {
+  Function *AggFunc = OG.OutlinedFunction;
----------------
Likewise, I think this can also be ArrayRef-ified.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1003
 
-  // Do the same for the other extracted functions
+  BasicBlock *NewBB;
   for (unsigned Idx = 1; Idx < CurrentGroup.Regions.size(); Idx++) {
----------------
I'd sink this into the loop to make it clear you don't have a loop-carried dependence on it.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1086
+void IROutliner::updateOutputMapping(OutlinableRegion &Region,
+                                     SetVector<Value *> &Outputs,
+                                     LoadInst *LI) {
----------------
Outputs should probably be `const`, or made into an `ArrayRef`, since it's not modified here.


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

https://reviews.llvm.org/D87296