[PATCH] D20268: [wip] Resolution-based LTO API.

[Peter Collingbourne via llvm-commits]

pcc added a comment.

> Also, it is a huge patch - suggest committing in a few different pieces:


Makes sense. Could also split it up into differential revisions if it makes it easier to review.


================
Comment at: include/llvm/LTO/Resolution/LTOBackend.h:106
@@ +105,3 @@
+  ///
+  /// This hook is called regardless of whether the backend is in-process.
+  typedef std::function<bool(const ModuleSummaryIndex &Index)>
----------------
tejohnson wrote:
> Although it is not called for the backend process (which also has a Config object per D21545). I do know what you are trying to say here - I think the distinction is that this is always invoked via LTO::run regardless of whether the backend is in-process.
> 
> Whereas the earlier hooks are only invoked from the backends themselves (regarless of whether invoked in process or in a separate backend process)
Clarified.

================
Comment at: lib/LTO/Resolution/LTO.cpp:167
@@ +166,3 @@
+  if (!Backend)
+    this->Backend = createInProcessThinBackend(thread::hardware_concurrency());
+}
----------------
tejohnson wrote:
> Can we avoid creating this if we are not going to perform ThinLTO? E.g. Create lazily in runThinLto if it is still null?
Maybe, but that sounds like a premature optimization to me.

================
Comment at: lib/LTO/Resolution/LTOBackend.cpp:32
@@ +31,3 @@
+    Hook = [=](size_t Task, Module &M) {
+      if (OldHook && !OldHook(Task, M))
+        return false;
----------------
tejohnson wrote:
> I'm confused by what is going on here with OldHook.
This is calling the hook provided by the linker, which still needs to run (e.g. in order to add common symbols). If the linker's hook returned false, we need to pass that result through.

================
Comment at: test/LTO/Resolution/X86/comdat.ll:14
@@ +13,3 @@
+; RUN:  -r=%t2.o,f1,l \
+; RUN:  -r=%t2.o,will_be_undefined, \
+; RUN:  -r=%t2.o,v1, \
----------------
tejohnson wrote:
> Does this need to be specified here? It looks like a suitable default (with none of the flags) will be constructed by default if not.
After thinking about it some more, I reckon that in order to reduce the possibility of mistakes, we probably don't want to implement a default resolution in `llvm-lto2`, nor should we accept resolutions for non-existent symbols. I have made that change to the test harness.

================
Comment at: test/LTO/Resolution/X86/comdat.ll:73
@@ -51,3 +72,3 @@
 
-; CHECK:      define weak_odr protected i32 @f1(i8*) comdat($c1) {
+; CHECK:      define weak_odr i32 @f1(i8*) comdat($c1) {
 ; CHECK-NEXT: bb10:
----------------
tejohnson wrote:
> What causes the difference w.r.t. gold here (protected vs not)?
Unlike the existing gold plugin, we do not resolve the visibility of each symbol and apply it to the combined module. It is unnecessary to do so because gold has already resolved the symbol's visibility using information provided by the plugin (i.e. `LDPV_*`) and will apply it to the final output file. The corresponding gold test (`test/LTO/Resolution/X86/comdat.ll`) shows that the symbol receives the correct visibility.

================
Comment at: test/tools/gold/X86/coff.ll:19
@@ -18,3 +18,3 @@
 
-; CHECK: define internal void @h() local_unnamed_addr {
+; CHECK: define internal void @h() {
 define linkonce_odr void @h() local_unnamed_addr {
----------------
tejohnson wrote:
> Why the loss of local_unnamed_addr?
LTO only tracks whether the symbol is global unnamed_addr. We do not track local_unnamed_addr for similar reasons as for visibility: it is the linker's job to keep track of whether the presence of that attribute can permit internalization (or, for Mach-O, auto-hide), and its presence in the IR shouldn't affect the generated code.

================
Comment at: test/tools/gold/X86/thinlto_alias.ll:24
@@ -19,1 +23,3 @@
+; for preempted symbols has not yet been implemented for the combined summary.
+; XFAIL: *
 
----------------
tejohnson wrote:
> Since thinBackend() now invokes thinLTOInternalizeModule, why is this failing?
The internalization done in thinLTOInternalizeModule for symbols that are not externally visible is separate from the internalization done for preempted alias targets (search for InternalLinkage in IRMover.cpp), which is not yet implemented for ThinLTO. I believe the practical effect here is that the linker will end up re-resolving the weak symbol from the resulting native object files. (This is similar to how we currently handle (non-ODR) weak or linkonce symbols, as we currently don't have a summary resolution that means "discard this symbol".)

================
Comment at: tools/gold/gold-plugin.cpp:730
@@ +729,3 @@
+    if (options::thinlto)
+      Backend = createInProcessThinBackend(options::Parallelism);
+    else
----------------
tejohnson wrote:
> Can we skip this if options::thinlto_index_only? Otherwise Backend is simply overwritten just below.
It seems more straightforward to just let it be overwritten.


http://reviews.llvm.org/D20268