[PATCH] D86977: [IRSim][IROutliner] Limit to extracting regions that only require inputs

[Andrew Litteken via Phabricator via llvm-commits]

AndrewLitteken added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/IROutliner.h:187
   /// can identify as having similarity, but are more complicated to outline.
-  struct InstructionAllowed
-      : public InstVisitor<InstructionAllowed, bool> {
+  struct InstructionAllowed : public InstVisitor<InstructionAllowed, bool> {
     InstructionAllowed() {}
----------------
plotfi wrote:
> Can these white space changes be in an NFC commit? 
I can just make them correct in the commit that introduces this section.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:266
+  for (Value *Input : CurrentInputs) {
+    assert(Input && "Have a nullptr as an input");
+    assert(C.getGVN(Input).hasValue() &&
----------------
plotfi wrote:
> Does this need to be a SetVector of Value pointers? Can it be references instead? You can drop the assert on nullptr then. 
Yes, this is from the CodeExtractor, so unless I reconstruct the SetVector, it has to stay as pointers :/.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:316-330
+  // resolution that adds too much complexity at this stage.
+  if (Outputs.size() > 0) {
+    Region.IgnoreRegion = true;
+    return;
+  }
+
+  // TODO: Support regions with sunken allocas: values whose lifetimes are
----------------
plotfi wrote:
> Why cant these conditional blocks be merged? ie:
> 
> 
> ```
>   if (Outputs.size() && ArgInputs.size() != OverallInputs.size()) {
>     Region.IgnoreRegion = true;
>     return;
>   }
> ```
They can be, they were just simplified for readability so I could included more detailed comments for why we were skipping these cases.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:335
+
+void IROutliner::findAddInputsOutputs(
+    Module &M, OutlinableRegion &Region) {
----------------
paquette wrote:
> Does `IgnoreRegion` have to be a member variable at all?
> 
> Maybe make this and `getCodeExtractorArguments` return a bool?
> 
> e.g.
> 
> ```
> return getCodeExtractorArguments(Region, Inputs, ArgInputs);
> ```
> 
> Then the caller of this function can just check whether or not this function succeeded via the bool?
That would probably work, and be a bit cleaner.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:519
+    // Create functions out of all the sections, and mark them as outlined.
+    OutlinedRegions.clear();
     for (OutlinableRegion *OS : CurrentGroup.Regions) {
----------------
jroelofs wrote:
> Referencing the moved-from object, though legal, seems strange. Maybe the two loops here ought to process the regions into differently named worklists. Since the first loop hasn't outlined them yet, maybe call that one `OutlineableRegions` and the second `OutlinedRegions`?
That's a better idea. And it will prevent a second move later on as well.


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

https://reviews.llvm.org/D86977