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

Tue May 24 19:15:37 PDT 2016

pcc added a comment.

In http://reviews.llvm.org/D20268#436125, @pcc wrote:

> > The information for each ThinLTO "backend-task" are packaged and forwarded to a custom LTOBackend
>
>
> Okay, just to make sure we are using the same terminology:
>
> - LTOBackend is the component that would have in-process and distributed implementations

This is what the new code calls ThinBackendProc. The user-level type is ThinBackend, which is a `std::function` that returns a ThinBackendProc which is a class with some virtual functions that handle the backend processing for an individual link.

> - ThinLTOProcessor is the component that would be used by clang and the in-process implementation

This is the function `lto::thinBackend()`.

> Correct?

> 

> I think ThinBackend would be a better name for the former because it would only be used for ThinLTO.

> 

> > if in-memory, each task take the result of the IPA and setup a ThinLTOProcessor (strawman name I just made up) that actually load the IR, performs the import, codegen, etc.

> 

> 

> That's what this code calls an LTOBackend (ThinLTOProcessor in your terminology), but I was going to split it up into free functions, just so it's clearer what's used for regular LTO, what's used for ThinLTO and what's used for both (no more misleading class names).

The backend interface is now just the `lto::backend()` function which runs the regular LTO backend, and `lto::thinBackend()` which runs the thin backend.

> 

> 

> > In the "distributed" mode, I assume the linker stops here and yield to the build system

> 

> 

> Yes, or in a distcc-style scenario it can wait for the distributed results to come back. I think the "waiting" can be done as a member function of the ThinBackend class as either a wait on the thread pool (for in-process), a waitpid (for distcc-style) or no wait (for custom build system packaging).

This is `ThinBackendProc::wait()`.

> 

> 

> > "Load this LTO memory buffer" (returns an LTOObjectFile), could be a free function?

> 

> 

> Sure. I think for now we'll want this to be a member function of LTO because of LLVMContext, but we can easily make it a free function later when the symbol table happens.

This is `InputFile::create()`. It takes an LTO parameter, which we can remove eventually, through which we get the LLVMContext.

> > "Iterate through the symbols for this LTOObjectFile: maybe a LTOSymbol class that exposes whatever level of information we would be exposing "per symbol". We can next think about laying out these informations in the bitcode as a symbol table that would be more efficient (i.e, not requiring much parsing/loading in memory). There's a PR that Rafael opened for that.

> 

> 

> I can see your point that exposing only the necessary information would make things a little easier to refactor later for the symbol table. I'll do that then.

This is `lto::InputFile::Symbol`.

================
Comment at: include/llvm/Transforms/IPO/LTOBackend.h:16
@@ +15,3 @@
+
+/// This class implements the "backend" phase of LTO or ThinLTO, specifically
+/// the part that takes IR and optimizes it, possibly after importing from other
----------------
pcc wrote:
> tejohnson wrote:
> > I'm a little confused by the layering between LTO and LTOBackend. Both say that they are for both regular LTO and for ThinLTO, but the only common handling in LTOBackend seems to be running the optimization pipeline. LTOBackend also contains a bunch of ThinLTO specific handling, some of which (like internalization) is done in the LTO class for the regular LTO case. I think this split may be clearer when this is merged with my patch that refactors the ThinLTO internalization and linkonce resolution, where there is a thin link portion (which should go in LTO) and a backend portion that operates on the index (which should go in Transforms/IPO, so LTOBackend). 
> > 
> > Maybe LTOBackend should be renamed to something like LTOOptimization to be clearer? I.e. it isn't doing a full backend, just the opt portion.
> Yeah, I think it'll make sense to make more of these into free functions, which would be either specific to regular LTO or specific to ThinLTO or shared (i.e. for the opt pipeline). I also want to look at moving some of the code into the CodeGen library so that it can actually do code generation as well.
> 
> At this point the current LTOBackend is pretty much just configuration so I want to make that into the LTOConfig I mentioned earlier to Mehdi and pass it into the free functions.
All the user-level configuration is now in `lto::Config`. It currently lives alongside the new LTO backend interface in CodeGen. I'm not really sure if that's the best place for it. I was thinking about
- creating a separate library for resolution-based LTO which would depend on nothing more than CodeGen so that it could be used by clang
- moving the LTO class and LTO backend code there
- making both depend on a Config.h header which would live in the same library and would define `lto::Config`

================
Comment at: include/llvm/Transforms/IPO/LTOBackend.h:76
@@ +75,3 @@
+  /// splitting the module.
+  ModuleHookFn PreCodeGenModuleHook;
+
----------------
pcc wrote:
> tejohnson wrote:
> > Why is this one declared here in LTOBackend and only used in derived class LTO? Seems like it should be moved to LTO?
> This will be moved into LTOConfig.
Done

================
Comment at: lib/LTO/LTO.cpp:237
@@ +236,3 @@
+
+void LTO::runThinLto(AddStreamFn AddStream, AddFileFn AddFile) {
+  if (ThinObjs.empty())
----------------
pcc wrote:
> tejohnson wrote:
> > Is it expected that AddFile is not used anywhere? If that is still TODO can you note that?
> Yes, this will be used for caching but not implemented yet. Maybe I'll just remove it, it'll be easy to add back later.
Removed

================
Comment at: lib/LTO/LTO.cpp:279
@@ +278,3 @@
+      if (OS)
+        WriteIndexToFile(CombinedIndex, *OS, &ModuleToSummariesForIndex);
+    } else {
----------------
pcc wrote:
> tejohnson wrote:
> > Can this be moved into the ThinBackendHook instead of returning an output stream and doing this here? It seems like the custom backend handling is being split into two places (here and in the ThinBackendHook). Also, why not pass in ImportLists[ModulePath] and let the custom handler figure out how to deal with them? I guess you are trying to keep the interface to just the task id and paths. But since the custom handler needs to understand ThinLTO anyway, it seems like it would be more intuitive for all the handling to be there.
> > 
> > Additionally, if the ThinBackendHook does something like launch the backend tasks itself, the individual index file needs to be written earlier.
> That all makes sense. Once we do that, all the processing for an individual task (for both distributed and in-process) can be moved into the ThinBackendHook. At that point, it wouldn't really be a hook but more of a "layer", so I guess I'll make this a class as I mentioned above.
This is now all part of the internal ThinBackendProc interface.

http://reviews.llvm.org/D20268