[PATCH] D26872: Outliner: Add MIR-level outlining pass

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

Thanks for pushing this, this is coming along nicely!

At this point I don't see any correctness or compiletime problems (with the comments below addressed).
Let's commit this and keep improving it in-tree. This should also make reviewing easier as we can have smaller follow-up patches.



================
Comment at: lib/CodeGen/MachineOutliner.cpp:782-783
+
+/// \brief Stores created outlined functions and the information needed to
+/// construct them.
+struct OutlinedFunction {
----------------
An instance of this only describes a single outlined function.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:800
+
+  // The number of instructions this function would save.
+  unsigned Benefit = 0;
----------------
doxygen `///`


================
Comment at: lib/CodeGen/MachineOutliner.cpp:844
+    bool WasInserted;
+    auto It = InstructionIntegerMap.end();
+    std::tie(It, WasInserted) =
----------------
I don't think you need to initialize this, it gets overwritten anyway in the next line.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:1225-1241
+  // Create the function name. This should be unique. For now, just hash the
+  // module name and include it in the function name plus the number of this
+  // function.
+  std::ostringstream NameStream;
+  size_t HashedModuleName = std::hash<std::string>{}(M.getName().str());
+  NameStream << "OUTLINED_FUNCTION" << HashedModuleName << "_" << OF.Name;
+
----------------
Hmm...

- This hash doesn't seem collision free. Someone having two files with the same name (maybe in two different projects that he links together later) may happen.
- Of course a collision shouldn't hurt as the linker will compare the contents anyway, but why even bother with a hash then?
- I think the linker will only try to merge functions with the same name but the function name(-hash) is currently based on the name not the contents of the function so I would expect this to be not helpful in most cases.

Maybe stay with the previous internal linking and try the LinkOnce tricks in a follow-up commit (where it is based on the contents).


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:10424-10425
+  if (MI.modifiesRegister(X86::RSP, &RI) || MI.readsRegister(X86::RSP, &RI) ||
+      MI.getDesc().hasImplicitUseOfPhysReg(X86::RSP) ||
+      MI.getDesc().hasImplicitDefOfPhysReg(X86::RSP))
+    return false;
----------------
This is surprising, checking the MCInstrDesc should not be necessary.
This is most probably a bug somewhere else in codegen, so there is nothing we can do here.

However I'd be good if you could find the time later to create a reproducer and file a PR about it, reading and writing registers without having operands for it looks like a bug waiting to happen elsewhere.


================
Comment at: test/CodeGen/X86/machine-outliner-basic.ll:1
+; RUN: llc -enable-machine-outliner -march=x86-64 < %s | FileCheck %s
+
----------------
Better use `-mtriple=x86_64--` instead of `-march` so we also force the operating system etc.


================
Comment at: test/CodeGen/X86/machine-outliner-basic.ll:24
+  store i32 1, i32* @x, align 4
+  ; CHECK: callq _OUTLINED_FUNCTION{{[0-9]+}}_0
+  store i32 1, i32* %2, align 4
----------------
You probably want to force this test to use the same outlined function everywhere. FileCheck allows assigning names and checking for repeated patterns:

```
; CHECK: callq [[_OUTLINED_FUNCTION[0-9]+_0:OUTLINEFUNC]]
...
; CHECK: callq [[OUTLINEFUNC]]
...
; CHECK-LABEL: [[OUTLINEFUNC]]:
```


================
Comment at: test/CodeGen/X86/machine-outliner-bb-boundaries.ll:1
+; RUN: llc -enable-machine-outliner -march=x86-64 < %s | FileCheck %s
+
----------------
You can probably merge the tests together into a single file as they are all about the same pass and use the same llc flags.


================
Comment at: test/CodeGen/X86/machine-outliner-bb-boundaries.ll:22
+
+; <label>:8:                                      ; preds = %0
+  ; CHECK: callq _OUTLINED_FUNCTION{{[0-9]+}}_0
----------------
I'd remove those standard dumping comments. If you actually care about the label give it a real name, if not the comment shouldn't be necessary either.


https://reviews.llvm.org/D26872