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

[Peter Collingbourne via llvm-commits]

pcc added a comment.

> 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
- ThinLTOProcessor is the component that would be used by clang and the in-process implementation

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).

> 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).

> "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.

> "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.

> So I wouldn't care about optimizing the API to handle the LLVMContext in particular.


Okay, I'll just put everything in a single LLVMContext for now.


================
Comment at: include/llvm/LTO/LTO.h:150
@@ +149,3 @@
+  /// A thin backend hook is called after the thin-link phase, before starting a
+  /// ThinLTO backend task.
+  ///
----------------
tejohnson wrote:
> This needs clarification - it isn't called before starting a backend task, it is called instead of launching backend tasks from here. Probably should be something like:
> "A thin backend hook is called after the thin-link phase, and used to implement custom backend handling, instead of the default ThinLTO backend task launching otherwise performed here."
This comment will probably be obsoleted by some refactoring that will replace ThinBackendHook with a ThinBackend class. That class will take a summary index to address your other comment.

================
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
----------------
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.

================
Comment at: include/llvm/Transforms/IPO/LTOBackend.h:76
@@ +75,3 @@
+  /// splitting the module.
+  ModuleHookFn PreCodeGenModuleHook;
+
----------------
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.

================
Comment at: lib/LTO/LTO.cpp:237
@@ +236,3 @@
+
+void LTO::runThinLto(AddStreamFn AddStream, AddFileFn AddFile) {
+  if (ThinObjs.empty())
----------------
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.

================
Comment at: lib/LTO/LTO.cpp:256
@@ +255,3 @@
+
+  std::vector<std::pair<size_t, std::unique_ptr<IRObjectFile>>> ThinBackends;
+
----------------
tejohnson wrote:
> Unused
Will remove.

================
Comment at: lib/LTO/LTO.cpp:279
@@ +278,3 @@
+      if (OS)
+        WriteIndexToFile(CombinedIndex, *OS, &ModuleToSummariesForIndex);
+    } else {
----------------
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.

================
Comment at: lib/Transforms/IPO/LTOBackend.cpp:63
@@ +62,3 @@
+
+  // FIXME: Internalize based on symbol resolutions.
+
----------------
tejohnson wrote:
> This will be based on the index, not symbol resolution (the symbol resolution will be used to update the index in the thin link stage, see D20290). Ditto for the upgradeLinkage above.
Yes, I meant symbol resolutions via the index. Will fix.


http://reviews.llvm.org/D20268