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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

I am all the way through except for looking at gold-plugin.c and the test changes (will get to those later this morning). Hopefully Mehdi will get a chance to look soon, but overall the interfaces look pretty good to me.


================
Comment at: include/llvm/CodeGen/LTOBackend.h:20
@@ +19,3 @@
+
+struct Config {
+  std::string CPU;
----------------
Needs description

================
Comment at: include/llvm/CodeGen/LTOBackend.h:109
@@ +108,3 @@
+/// Runs a ThinLTO backend.
+bool thinBackend(Config &C, StringRef TheTriple, const Target *TheTarget,
+                 size_t Task, AddStreamFn AddStream, Module &M,
----------------
Document return value here and above for backend()

================
Comment at: include/llvm/LTO/LTO.h:224
@@ +223,3 @@
+  /// Add an input file to the LTO link, using the provided symbol resolutions.
+  /// The symbol resolutions must appear in the enumeration order given by
+  /// InputFile::symbols().
----------------
Wondering if it would be better (less fragile and easier to set up in the caller) to pass a map of Symbol* to resolutions, instead of an ArrayRef with an implied ordering.

================
Comment at: include/llvm/LTO/LTO.h:229
@@ +228,3 @@
+  /// Returns an upper bound on the number of tasks that the client may expect.
+  /// This may only be called after all IR object files have been added. For a
+  /// full description of tasks see include/llvm/CodeGen/LTOBackend.h.
----------------
Is it worth making it an error to call add() after getMaxTasks has been called? (i.e. via a flag set by getMaxTasks and checked by add)

================
Comment at: include/llvm/LTO/LTO.h:254
@@ +253,3 @@
+
+  struct GlobalResolution {
+    /// The unmangled name of the global.
----------------
Needs a description

================
Comment at: include/llvm/LTO/LTO.h:315
@@ +314,3 @@
+
+struct SymbolResolution {
+  SymbolResolution()
----------------
Needs description.

================
Comment at: lib/CodeGen/LTOBackend.cpp:45
@@ +44,3 @@
+
+bool opt(Config &C, StringRef TheTriple, const Target *TheTarget, size_t Task,
+         Module &M, bool IsThinLto) {
----------------
should be static

================
Comment at: lib/CodeGen/LTOBackend.cpp:77
@@ +76,3 @@
+
+bool codegen(Config &C, StringRef TheTriple, const Target *TheTarget,
+             AddStreamFn AddStream, size_t Task, Module &M) {
----------------
should be static

================
Comment at: lib/CodeGen/LTOBackend.cpp:93
@@ +92,3 @@
+
+bool splitCodeGen(Config &C, StringRef TheTriple, const Target *TheTarget,
+                  AddStreamFn AddStream,
----------------
should be static

================
Comment at: lib/CodeGen/LTOBackend.cpp:160
@@ +159,3 @@
+
+  // FIXME: Apply symbol resolutions here.
+  for (Function &F : M)
----------------
Should be able to invoke thinLTOResolveWeakForLinkerModule now, refactored in r270698.

================
Comment at: lib/CodeGen/LTOBackend.cpp:173
@@ +172,3 @@
+
+  // FIXME: Internalize based on summary info.
+
----------------
Should be able to invoke thinLTOInternalizeModule now, refactored in r270698.

================
Comment at: lib/LTO/LTO.cpp:69
@@ +68,3 @@
+  if (!Backend)
+    this->Backend = createInProcessThinBackend(thread::hardware_concurrency());
+}
----------------
Need a way to control the parallelism via option.

Edit: I see that when we create this via gold, we pass in a Backend that uses the plugin's jobs option to set the parallelism. When would we want to create one here?

================
Comment at: lib/LTO/LTO.cpp:376
@@ +375,3 @@
+  CombinedIndex.collectDefinedGVSummariesPerModule(
+      ModuleToDefinedGVSummaries);
+
----------------
Can we pass this in to the ThinBackendProc so that it doesn't need to be called again when constructing a WriteIndexesThinBackend?

================
Comment at: lib/LTO/LTO.cpp:394
@@ +393,3 @@
+    ++Task;
+    ++Partition;
+  }
----------------
Do we need to remember the mapping between Task and Partition for later setting up the isPrevailing callback of thinLTOResolveWeakForLinkerInIndex?

================
Comment at: tools/llvm-lto2/LLVMBuild.txt:1
@@ -1,2 +1,2 @@
-;===- ./lib/CodeGen/LLVMBuild.txt ------------------------------*- Conf -*--===;
+;===- ./tools/llvm-lto/LLVMBuild.txt ----------------------------*- Conf -*--===;
 ;
----------------
Wrong path name

================
Comment at: tools/llvm-lto2/llvm-lto2.cpp:1
@@ +1,2 @@
+#include "llvm/LTO/LTO.h"
+#include "llvm/Object/IRObjectFile.h"
----------------
Is this tool temporary, while we are transitioning to new API? Are there any tests that use it? I didn't see any.


http://reviews.llvm.org/D20268