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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Looks like it is getting very close. A few comments/questions below.

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

1. Move existing LTO.{cpp,h} to Resolution subdirectory
2. Add LTOBackend.cpp, changes to LTO.{cpp,h}, and llvm-lto2 + tests
3. Changes to gold-plugin.cpp to use new interfaces


================
Comment at: include/llvm/LTO/Resolution/LTO.h:321
@@ +320,3 @@
+  // internalization decisions either directly to the module (for regular LTO)
+  // or to the combined index (for ThinLTO; FIXME: not implemented yet).
+  struct GlobalResolution {
----------------
Is the ThinLTO support still TODO? It looks like this can be removed as I see it here now.

================
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)>
----------------
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)

================
Comment at: include/llvm/LTO/Resolution/LTOBackend.h:111
@@ +110,3 @@
+
+  Error addSaveTemps(std::string OutputFileName);
+};
----------------
Add doxygen comment

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

================
Comment at: lib/LTO/Resolution/LTO.cpp:370
@@ +369,3 @@
+
+  StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries;
+
----------------
Could move this to parent class since it is in both derived classes

================
Comment at: lib/LTO/Resolution/LTO.cpp:561
@@ +560,3 @@
+
+  size_t Task = ParallelCodeGenParallelismLevel;
+  size_t Partition = 1;
----------------
Can you document why we are initializing Task to ParallelCodeGenParallelismLevel? I thought earlier it said somewhere that regular LTO (parallel or not) was TaskId 0 and ThinLTO started at TaskId 1.

================
Comment at: lib/LTO/Resolution/LTOBackend.cpp:32
@@ +31,3 @@
+    Hook = [=](size_t Task, Module &M) {
+      if (OldHook && !OldHook(Task, M))
+        return false;
----------------
I'm confused by what is going on here with OldHook.

================
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, \
----------------
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.

================
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:
----------------
What causes the difference w.r.t. gold here (protected vs not)?

================
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 {
----------------
Why the loss of local_unnamed_addr?

================
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: *
 
----------------
Since thinBackend() now invokes thinLTOInternalizeModule, why is this failing?

================
Comment at: tools/gold/gold-plugin.cpp:730
@@ +729,3 @@
+    if (options::thinlto)
+      Backend = createInProcessThinBackend(options::Parallelism);
+    else
----------------
Can we skip this if options::thinlto_index_only? Otherwise Backend is simply overwritten just below.

================
Comment at: tools/llvm-lto2/llvm-lto2.cpp:2
@@ +1,3 @@
+#include "llvm/LTO/Resolution/LTO.h"
+#include "llvm/Object/IRObjectFile.h"
+#include "llvm/Support/TargetSelect.h"
----------------
Should probably add a comment at the top of the file about why this exists then.

================
Comment at: tools/llvm-lto2/llvm-lto2.cpp:20
@@ +19,3 @@
+    "r",
+    cl::desc("Specify a symbol resolution: filename,symbolname,resolution"),
+    cl::ZeroOrMore);
----------------
Need to describe format of "resolution" (e.g. 'p','l','x' and meanings). Also, it would be good to mention the default resolution for anything not specified here (which from the code it appears to be non-p, non-l, and non-x).


http://reviews.llvm.org/D20268