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

[Peter Collingbourne via llvm-commits]

pcc added a comment.

> Just started to look through the new patch. A high level question - for the distributed backend case, is the idea that clang would invoke LTOBackend:: runThinLtoBackendThread directly, instead of setting up a pass pipeline itself?


LTOBackend::runThinLtoBackend, but yes.

> Were you able to get this patch to work with the clang side distributed backend path?


I was going to look at that next after I flesh out the test harness a little more.

> That was not my understanding, I'd expect *only* the linker to interact with this API.


My understanding was that the linker would use the LTO class, while the LTOBackend class would be shared between clang and whichever part of the LTO class actually launches the in-process backends.


================
Comment at: include/llvm/LTO/LTO.h:51
@@ +50,3 @@
+public:
+  LTO(LLVMContext &Ctx);
+
----------------
joker.eph wrote:
> 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).
So on IRC you mentioned that we may want to conditionally create contexts up front for ThinLTO backends. That seems like a reasonable enough reason to me to introduce another class that could wrap IRObjectFile. Something like this seems like it would do:
```
class LTO {
...
public:
  class ObjectFile {
     std::unique_ptr<LLVMContext> OwnedContext;
     std::unique_ptr<IRObjectFile> Obj;
  public:
    range symbols() {
      // move LTO::symbols here
    }
  };
};
```

================
Comment at: include/llvm/LTO/LTO.h:56
@@ +55,3 @@
+  unsigned ParallelCodeGenParallelismLevel = 1;
+  unsigned ThinLTOParallelismLevel = 1;
+
----------------
joker.eph wrote:
> 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.
Okay, I'll see if I can create an options struct and have that be a field of LTOBackend or something like that.

================
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:
> 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.
Okay, let's get a little concrete here, because I'm not sure exactly what you mean. Basically, we have something in the implementation that looks like:
```
void LTO::runThinLto() {
  // thin link
  for each object {
    if (distributed) {
      // create individual module index
      // write individual module index
    } else {
      // launch backend thread
    }
  }
}
```
Are you saying that you want `LTO::runThinLto` to be moved to a "backend" class which could be configured to be distributed for example?


http://reviews.llvm.org/D20268