[PATCH] D106440: [IROutliner] Change Prioritization of Outlining to honor cost model

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

Do you have data on the code size impact of this change?



================
Comment at: llvm/include/llvm/Transforms/IPO/IROutliner.h:213
+
+  /// For the \ref Regions, we look at every Value.  If it is a constant,
+  /// we check whether it is the same in Region.
----------------
Should this say OutlinableRegion?


================
Comment at: llvm/include/llvm/Transforms/IPO/IROutliner.h:259
+  bool
+  compatibleWithOutlined(OutlinableRegion &Region);
+
----------------
This seems to have an incorrect comment.


================
Comment at: llvm/include/llvm/Transforms/IPO/IROutliner.h:375
+  /// The memory allocator used to allocate the OutlinableGroups.
+  SpecificBumpPtrAllocator<OutlinableGroup> GroupAllocator;
+  
----------------
why?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:69
 
+
   if (Candidate->end()->Inst !=
----------------
Unnecessary whitespace change?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1279
+  for (unsigned Idx = StartIdx; Idx <= EndIdx; Idx++)
+    if (Outlined.contains(Idx)) {
+      return false;
----------------
clang format braces


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1287
+    Instruction *NewEndInst =
+        Region.Candidate->backInstruction()->getNextNonDebugInstruction();
+    IRInstructionData *NewEndIRID = new (InstDataAllocator.Allocate())
----------------
Might as well pull this above the `if` to avoid recalculating it.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1311
+
+  return !BadInst;
+}
----------------
Probably don't need the variable here?

Just

```
return any_of(...);
```


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1643
   for (SimilarityGroup &CandidateVec : SimilarityCandidates) {
-    OutlinableGroup CurrentGroup;
+    OutlinableGroup *CurrentGroup =
+        new (GroupAllocator.Allocate()) OutlinableGroup;
----------------
why?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106440