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

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: include/llvm/LTO/LTO.h:51
@@ +50,3 @@
+public:
+  LTO(LLVMContext &Ctx);
+
----------------
joker.eph wrote:
> I don't expect a single context here, describe what you have in mind.
This context is used for regular LTO; created `IRObjectFile`s are expected to belong to this context in case they are needed for regular LTO. ThinLTO backend tasks use separate contexts (see `LTO::runThinLtoBackendThread`).

================
Comment at: include/llvm/LTO/LTO.h:56
@@ +55,3 @@
+  unsigned ParallelCodeGenParallelismLevel = 1;
+  unsigned ThinLTOParallelismLevel = 1;
+
----------------
joker.eph wrote:
> Lack of encapsulation: I'd rather create a nested `Options` class/struct (or a global `LTOOptions` one) and pass it to `run()`
That won't work, we need `ParallelCodeGenParallelismLevel` to calculate `getMaxTasks`. I also don't think it's worth complicating the implementation by passing an options struct around.

================
Comment at: include/llvm/LTO/LTO.h:141
@@ +140,3 @@
+
+  /// A combined index hook is called after all module indices have been
+  /// combined. It can be used to implement -save-temps for the combined index.
----------------
joker.eph wrote:
> Not sure what "indices" mean in this context.
Plural of "module index", but I think I need "indexes" here, since according to https://en.wiktionary.org/wiki/indices this sense of "index" cannot be pluralised as "indices".

================
Comment at: include/llvm/LTO/LTO.h:155
@@ +154,3 @@
+  /// it could copy the index, input file and import files to a remote machine
+  /// and run the backend task there.
+  ///
----------------
joker.eph wrote:
> I'm not sure I'm convinced this is the right level to implement that, i.e. delegating to the linker the orchestration.
I think in most cases the linker wouldn't be implementing this hook directly. What I was thinking was that for this hook I would like the API to provide a way to install "prefabricated" hooks in the same way that `addSaveTemps` would install a prefabricated save-temps hook.

For example, we could have an `addWriteIndexesBackend()` function that would install a hook to write the index and import lists to the file system as the gold plugin is currently doing, and an `addLaunchProcessBackend(StringRef CommandLine)` function that would install a hook to create a process that would actually launch the backend (the idea is that `CommandLine` would be something like `distcc`).

================
Comment at: include/llvm/LTO/LTO.h:157
@@ +156,3 @@
+  ///
+  /// The caller must set up and return an output stream to write the index to.
+  typedef std::function<std::unique_ptr<raw_ostream>(
----------------
joker.eph wrote:
> Do you mean callee? I don't understand how this API works.
Sorry, yes, I meant "callee".

================
Comment at: include/llvm/LTO/LTO.h:159
@@ +158,3 @@
+  typedef std::function<std::unique_ptr<raw_ostream>(
+      size_t Task, StringRef InputFile, ArrayRef<StringRef> ImportFiles)>
+      ThinBackendHookFn;
----------------
joker.eph wrote:
> `StringRef InputFile` seems to assume that  there is an actual path to a file, while it could be a buffer in memory.
I discussed this offline with Teresa before I implemented this. At first I was uncomfortable with using file paths here, but I think in many cases the linker is going to have a file path available. File paths also make it easier to implement the hook if it uses an external program.

The obvious exception is when the object is embedded within an archive, but in that case the build system can still arrange to make file paths available in some cases. For example, with gold or lld the build system can use `--start-lib`/`--end-lib` or thin archives. (There's another possible exception: when the IR is embedded in a native object.)

The solution for the embedded cases that we considered was to write the extracted object file to either the supplied cache directory or somewhere in  `$TMP` if no cache was supplied. Then that path can be passed here.

As one alternative, we may want to instead pass (file path, byte range) pairs here. Then the callee can extract the files itself, or pass them directly to the program that launches the backends if it is known to support byte ranges.

================
Comment at: include/llvm/LTO/LTO.h:167
@@ +166,3 @@
+
+  LLVMContext &getContext() const { return Ctx; }
+
----------------
joker.eph wrote:
> I don't expect to see this here, what is the use case?
In order to create an `IRObjectFile` that can be passed to `add`, we will need a context. This is a convenience to clients so that they only need an `LTO` object in order to create an `IRObjectFile` and add it to `LTO`.


http://reviews.llvm.org/D20268