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

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: include/llvm/LTO/LTO.h:26
@@ +25,3 @@
+  unsigned DefinitionInLinkageUnit : 1;
+
+  /// The definition of this symbol is visible outside of the LTO unit.
----------------
Thinking about ThinLTO, how do you avoid GOT/PLT accesses for symbols defined in other modules? These attributes seems only relevant for access within the same module, right?

================
Comment at: include/llvm/LTO/LTO.h:49
@@ +48,3 @@
+  }
+
+  class symbol_iterator {
----------------
What is specific to LTO in "iterating over the symbol in an object file"?
I'd like to avoid a large bloated interface here.

================
Comment at: include/llvm/LTO/LTO.h:89
@@ +88,3 @@
+           DiagnosticHandlerFunction DiagHandler);
+
+  /// The following callbacks deal with tasks, which normally represent the
----------------
The point is about splitting the API in multiple abstractions instead of a big blob here.
I was planning to have an LTOModule that expose the possibility for the linker to iterate over symbols, keep a reference to a symbols, and update the symbol resolution using the symbol reference directly.
The linker can mark the symbol resolution the first time it sees it in many cases.



================
Comment at: include/llvm/LTO/LTO.h:96
@@ +95,3 @@
+  /// entire pipeline:
+  /// - A task may be split into multiple tasks. For example, parallel code
+  ///   generation will be represented as a single task during optimization,
----------------
I see, however instead of a callback that callback in the callback .... model, I'd rather split it so that:

- linker calls run()
- the (Thin)LTO processing call something like `ObjectFile allocateNewObjectFile()`
- the //ObjectFile// offers a `getStream()` interface, as well as a `finalize()` or similar to signal completeness.

================
Comment at: include/llvm/LTO/LTO.h:118
@@ +117,3 @@
+  typedef std::function<void(size_t Task, CodeGeneratorFn CG)> AddStreamFn;
+
+  /// This type defines a file callback. The file callback passed to run() is
----------------
I'll wait to see something complete then, I'm worried it's not gonna easy to reconcile in a nice way though.

================
Comment at: include/llvm/LTO/LTO.h:123
@@ +122,3 @@
+  ///
+  /// File callbacks must be thread safe.
+  typedef std::function<void(size_t Task, StringRef S)> AddFileFn;
----------------
Can you elaborate what you mean? I don't really what you mean, so I don't get the need for task numbering

================
Comment at: include/llvm/LTO/LTO.h:124
@@ +123,3 @@
+  /// File callbacks must be thread safe.
+  typedef std::function<void(size_t Task, StringRef S)> AddFileFn;
+
----------------
I forgot to ask: why the "File" version? Why not passing the file as a "stream"?

================
Comment at: include/llvm/LTO/LTO.h:138
@@ +137,3 @@
+
+  /// Runs the LTO pipeline. This function calls the supplied AddStream and
+  /// AddFile functions to add native object files to the link.
----------------
These should all be moved to a different layer somehow.


http://reviews.llvm.org/D20268