[PATCH] D26872: Outliner: Add MIR-level outlining pass
Sean Silva via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 23 18:45:51 PST 2017
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
More information about the llvm-commits
mailing list