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

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: include/llvm/LTO/LTO.h:51
@@ +50,3 @@
+public:
+  LTO(LLVMContext &Ctx);
+
----------------
pcc wrote:
> 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`).
That's not satisfactory: now whoever creates the IRObjectFile needs to know about the partitioning.
Also, it does not explain why you pass it here (one can always peak into the IRObjectFile to see which context is in use).

================
Comment at: include/llvm/LTO/LTO.h:56
@@ +55,3 @@
+  unsigned ParallelCodeGenParallelismLevel = 1;
+  unsigned ThinLTOParallelismLevel = 1;
+
----------------
pcc wrote:
> 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.
This is not satisfactory from an API point of view. We need another solution.

================
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.
----------------
pcc wrote:
> 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".
Oh, I see, I don't think we ever named the individual module summaries "index", but only used this term for "combined index". That's why I was confused with what it was referring to.

================
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.
+  ///
----------------
pcc wrote:
> 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`).
Ok, injecting the implementation for the backend, with pre-defined implementation available looks like a good solution to me.

Why does it have to be ThinLTO specific? 

I'd expect the API in this class to deal with a layer that I consider as a "frontend" to talk with the linker, exposing only what the linker need to know. Any other details should not be exposed here, ideally we wouldn't need to see any LTO/ThinLTO/... distinction here. 
So yes we need to expose an API for the client (linker) to plug a backend, the backend being configured separately. Right now the decoupling between the "frontend" API and the backend does not seem yet totally split.

================
Comment at: include/llvm/LTO/LTO.h:167
@@ +166,3 @@
+
+  LLVMContext &getContext() const { return Ctx; }
+
----------------
pcc wrote:
> 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`.
cf ctor comment, we need to rethink the flow.


http://reviews.llvm.org/D20268