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

[Sean Silva via Phabricator via llvm-commits]

silvas added inline comments.


================
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;
+
----------------
MatzeB wrote:
> 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).
> Of course a collision shouldn't hurt as the linker will compare the contents anyway, but why even bother with a hash then?

No. linkonce_odr requires that if the name matches then the contents are interchangeable, since one gets selected arbitrarily. So for correctness the hash must be collision-free.

(see also the discussion in D29512 which also involves finding a stable "name" for the TU)

Also, I don't see the point of doing this. The linker's content-based deduplication ("ICF") should handle this case without caring about the name. If you want to use the linker's comdat/linkonce (i.e. name-based) deduplication then you can just use the function's contents as the name (mangling away NUL bytes), or a *strong* hash (collisions are a correctness problem).

Presumably, if users are using this pass, then they care about code size and so they are likely to have ICF enabled already. So I don't see the point of doing this linkage trick.


================
Comment at: test/CodeGen/X86/machine-outliner-interprocedural.ll:8
+define i32 @foo() #0 {
+  ; CHECK-LABEL: _foo:
+  %1 = alloca i32, align 4
----------------
The leading underscore here is darwin-specific. Add an explicit triple to avoid this (otherwise non-Darwin bots will break).


https://reviews.llvm.org/D26872