[PATCH] D69446: [llvm][MachineOutliner] Add support for repeating machine outliner N times.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

This is pretty cool, thanks for working on this.

This needs a couple tests. A few things I can think of:

1. Show that the outliner will stop when there are no changes, even if you haven't run the specified number of rounds
2. Show that adding extra iterations actually produces more outlining
3. Show what happens when you outline from an outlined function

I think that the easiest way to test (1) is to add some debug output which we can test. E.g. "did not outline on iteration <iteration> out of <number of iterations>"

Also, do you think you could test the code size + compile time impact of this patch on CTMark at -Oz for AArch64? I think that two or three data points would be sufficient. Say, 1 round versus 2 rounds versus 5 rounds versus 10 rounds.

(If it turns out that compile time is an issue, I suspect we might be able to save *some* time by keeping track of which functions and basic blocks can never be outlined from, for example.)

It would also be nice to know how many rounds on average this provides benefit for. I think 1 iteration is the correct default; this is more for my own curiosity.



================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:100
+static cl::opt<unsigned> Iterations(
+    "outlining-iteration", cl::init(1), cl::Hidden,
+    cl::desc("Total number of outlining iterations to perform"));
----------------
Maybe "outlining-iterations" to imply that it's something plural?


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:100
+static cl::opt<unsigned> Iterations(
+    "outlining-iteration", cl::init(1), cl::Hidden,
+    cl::desc("Total number of outlining iterations to perform"));
----------------
paquette wrote:
> Maybe "outlining-iterations" to imply that it's something plural?
What should happen if this is set to 0? Is there a way to specify a minimum value here?


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:903
   /// Construct a suffix tree on the instructions in \p M and outline repeated
-  /// strings from that tree.
+  /// strings from that tree. This will be run doOutline on M Iterations times.
   bool runOnModule(Module &M) override;
----------------
We can probably just remove the entire comment from runOnModule now, since it would all be folded into the comment on `doOutline`


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:907
+  /// Construct a suffix tree on the instructions in \p M and outline repeated
+  /// strings from that tree. This will do outlining on M once.
+  bool doOutline(Module &M);
----------------
\p M


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1438
 
+  if (Iterations > 1) {
+    bool Changed = false;
----------------
I don't think this `if` is necessary. Since `Iterations` is 1 by default, the loop should only run once when `Iterations` is not specified.


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1441-1445
+      if (doOutline(M))
+        Changed = true;
+      else
+        return Changed;
+    return Changed;
----------------
I think this can be simplified:

```
for(...)
  if (!doOutline(M))
    return false;
return true;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69446