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

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/IROutliner.h:185
+  /// Check whether an OutlinableRegion is incompatible with code already
+  /// outlined, that is whether there are overlapping instructions, code that
+  /// has not been recorded added to the instructions.
----------------
Break up run-on?


================
Comment at: llvm/include/llvm/Transforms/IPO/IROutliner.h:191
+  /// \returns whether the region can safely be outlined.
+  bool compatibleWithOutlined(OutlinableRegion &Region);
+
----------------
- `isCompatibleWithAlreadyOutlinedCode` might be a more descriptive name.
- Does this function modify `Region`? Should it be const?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:277
   // each division instruction.
-  for (Instruction &I : *StartBB) {
+  for (IRInstructionData &ID : *this->Candidate) {
+    Instruction &I = *ID.Inst;
----------------
is it actually necessary to use `this` here?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:278
+  for (IRInstructionData &ID : *this->Candidate) {
+    Instruction &I = *ID.Inst;
+    InstructionCost IndividualBen;
----------------
Is `ID` a pointer? Can you loop over it like

```
for (IRInstructionData *ID ... )
```

to make that clear?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:279
+    Instruction &I = *ID.Inst;
+    InstructionCost IndividualBen;
     switch (I.getOpcode()) {
----------------
Why is this variable necessary?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1353
+
+  for (unsigned Idx = StartIdx; Idx <= EndIdx; Idx++)
+    if (Outlined.contains(Idx))
----------------
Comment?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1357
+
+  Instruction *RealEndInstruction =
+      Region.Candidate->backInstruction()->getNextNonDebugInstruction();
----------------
Comment?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1357
+
+  Instruction *RealEndInstruction =
+      Region.Candidate->backInstruction()->getNextNonDebugInstruction();
----------------
paquette wrote:
> Comment?
What happens if this is `nullptr`?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1371
+
+  return !any_of(*IRSC, [this](IRInstructionData &ID) {
+    // We check if there is a discrepancy between the InstructionDataList
----------------
`none_of`?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1382
+      return true;
+    return !this->InstructionClassifier.visit(ID.Inst);
+  });
----------------
Do you need `this` here?

also you can probably just write

```
return (std::next(...)->Inst != ID.Inst...) || !InstructionClassifier.visit(ID.Inst);
```


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1711
+  // each of the following for loops to avoid making an allocator.
+  std::vector<OutlinableGroup> PotentialGroups(SimilarityCandidates.size());
 
----------------
Would this be better as a SmallVector?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1719
   for (SimilarityGroup &CandidateVec : SimilarityCandidates) {
-    OutlinableGroup CurrentGroup;
+    OutlinableGroup *CurrentGroup = &PotentialGroups[PotentialGroupIdx++];
 
----------------
Why not grab this as a reference rather than as a pointer? That would probably reduce the size of this patch significantly.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1760
         OutlinedRegions.push_back(OS);
-      else
-        OS->reattachCandidate();
+      OS->reattachCandidate();
     }
----------------
This could use a comment?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1807
+  if (NegativeCostGroups.size() > 1)
+    llvm::stable_sort(NegativeCostGroups, [](const OutlinableGroup *LHS,
+                                             const OutlinableGroup *RHS) {
----------------
I think you're using the llvm namespace, and `std::stable_sort` doesn't work with ranges, so I think you can just use

```
if (...)
  stable_sort(...)
```


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1818
+    for (OutlinableRegion *Region : CurrentGroup.Regions) {
+      if (!compatibleWithOutlined(*Region))
+        continue;
----------------
Comment?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1857
     for (OutlinableRegion *OS : CurrentGroup.Regions) {
+      std::vector<BasicBlock *> BE = {OS->StartBB};
+      OS->CE = new (ExtractorAllocator.Allocate())
----------------
Can this use a `SmallVector`, or will the CodeExtractor not accept that?


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

https://reviews.llvm.org/D106440