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

Fri May 20 08:26:44 PDT 2016

I added a bunch of comments via Phab this morning, but for some reason I am
not seeing an email sent out! I don't know how to force it to send the
email, but I have copied the comments below. Sorry for the crappy
formatting, I don't know how to copy it in with good context, but you can
look in Phab for the full context here http://reviews.llvm.org/D20268#435499
.

include/llvm/LTO/LTO.h
150
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."
include/llvm/Transforms/IPO/LTOBackend.h
16
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.
76
Why is this one declared here in LTOBackend and only used in derived class
LTO? Seems like it should be moved to LTO?
lib/LTO/LTO.cpp
237
Is it expected that AddFile is not used anywhere? If that is still TODO can
you note that?
256
Unused
279
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.
lib/Transforms/IPO/LTOBackend.cpp
63
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.

On Thu, May 19, 2016 at 9:52 PM, Mehdi AMINI <mehdi.amini at apple.com> wrote:

> joker.eph added a comment.
>
> Haven't had much time to look at anything in the implementation yet, as
> what seems important to me is the layering and separation of component. I'd
> expect anything the linker needs to setup / interact with to be concentrate
> in some place (lib/LTO or equivalent). What I'd try to avoid is exposing
> LLVM internals that are not relevant to the linker (like the LLVMContext
> for instance).
>
> Below is a quick brain-dump of a high-level view of how I'd expect the
> various layers of the LTO machinery to be laid-down:
>
> - A common sequential "thin" phase that performs IPA over the summary, and
> compute all the information needed by each backend task.
> - The information for each ThinLTO "backend-task" are packaged and
> forwarded to a custom LTOBackend implementation (which component/level
> handles the threading is fuzzy for now, but both scheme could be threaded I
> think):
>   - 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.
>   - if "distributed", each task that takes the result of the IPA and
> serialize it in a "thinlto-package" for the distributed build system.
>
> In the "distributed" mode, I assume the linker stops here and yield to the
> build system, which distributes the "thinlto-package". The distributed job
> run clang, which unserialize the "thinlto-package" and setup a
> `ThinLTOProcessor` the same way the in-memory backend does.
>
> Hopefully the part that setup the pipeline/targetmachine/codegen/... will
> be common between regular LTO and ThinLTO individual tasks.
> Not much of this needs to be exposed to the linker though. Let's keep the
> API exposed to the linker as small as possible.
> Some parts are fairly "easy":
>
> - "Load this LTO memory buffer" (returns an `LTOObjectFile`), could be a
> free function? Along these lines: `std::unique_ptr<LTOObjectFile>
> createLTOObjectFile(MemBufferRef Buffer)`. This can be lazy (i.e. don't do
> anything but verifying the bitcode signature).
> - "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. Ultimately we shouldn't need to load a Module and
> setup a LLVMContext to get the symbol names, their linkage type, their
> section, etc. So I wouldn't care about optimizing the API to handle the
> LLVMContext in particular.
> - Set the `SymbolResolution` for every `LTOSymbol`.
>
> This is somehow covered in this patch.
>
> What does not seem nailed down yet (or is not clear to me at least, maybe
> I need to start looking closer at the implementation) is how to setup a
> flavor of LTOBackend: with/without distributed build for instance,
> providing the appropriate configuration, diagnostic handler, and hooks (for
> adding a stream and such).
>
> (As a general comment: free functions are convenient (no hidden state) but
> they'd require to pass a diagnostic handler all the time which is annoying)
>
>
> http://reviews.llvm.org/D20268
>
>
>
>

-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160520/6d5329dd/attachment.html>