[PATCH] D86975: [IRSim][IROutliner] Adding the extraction basics for the IROutliner.

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/IROutliner.h:174
+  /// The memory allocator used to allocate the OutlinableRegions and
+  // CodeExtractors.
+  BumpPtrAllocator OutlinerAllocator;
----------------



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:238
+  // instruction.
+  for (Instruction &I : *RewrittenBB)
+    if (CallInst *CI = dyn_cast<CallInst>(&I))
----------------
Is it guaranteed that we'll find precisely one CallInst in this search? Should there be assertions to that effect?

Also, is it worth adding a `break` in the inner `if` to early exit once it has been found?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:277
+    // Create a CodeExtractor for each outlinable region.
+    std::vector<OutlinableRegion *> OutlinedRegions;
+    for (OutlinableRegion *OS : CurrentGroup.Regions) {
----------------
Can sink this to where the `.clear()` call is, since it's not used until then.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:303
+
+    CurrentGroup.Regions = OutlinedRegions;
+  }
----------------
should make the copy cheaper


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:852
 
+  if (EnableIROutliner)
+    MPM.add(createIROutlinerPass());
----------------
Out of curiosity, how did you decide that this was the optimal spot in the pass stack for it?


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

https://reviews.llvm.org/D86975