<div dir="ltr">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 <a href="http://reviews.llvm.org/D20268#435499">http://reviews.llvm.org/D20268#435499</a>.<div><br></div><div><div>include/llvm/LTO/LTO.h</div><div>150</div><div>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:</div><div>"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."</div><div>include/llvm/Transforms/IPO/LTOBackend.h</div><div>16</div><div>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).</div><div><br></div><div>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.</div><div>76</div><div>Why is this one declared here in LTOBackend and only used in derived class LTO? Seems like it should be moved to LTO?</div><div>lib/LTO/LTO.cpp</div><div>237</div><div>Is it expected that AddFile is not used anywhere? If that is still TODO can you note that?</div><div>256</div><div>Unused</div><div>279</div><div>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.</div><div><br></div><div>Additionally, if the ThinBackendHook does something like launch the backend tasks itself, the individual index file needs to be written earlier.</div><div>lib/Transforms/IPO/LTOBackend.cpp</div><div>63</div><div>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.</div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 19, 2016 at 9:52 PM, Mehdi AMINI <span dir="ltr"><<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">joker.eph added a comment.<br>
<br>
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).<br>
<br>
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:<br>
<br>
- A common sequential "thin" phase that performs IPA over the summary, and compute all the information needed by each backend task.<br>
- 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):<br>
  - 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.<br>
  - if "distributed", each task that takes the result of the IPA and serialize it in a "thinlto-package" for the distributed build system.<br>
<br>
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.<br>
<br>
Hopefully the part that setup the pipeline/targetmachine/codegen/... will be common between regular LTO and ThinLTO individual tasks.<br>
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.<br>
Some parts are fairly "easy":<br>
<br>
- "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).<br>
- "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.<br>
- Set the `SymbolResolution` for every `LTOSymbol`.<br>
<br>
This is somehow covered in this patch.<br>
<br>
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).<br>
<br>
(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)<br>
<br>
<br>
<a href="http://reviews.llvm.org/D20268" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20268</a><br>
<br>
<br>
<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Teresa Johnson |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"> 408-460-2413</td></tr></tbody></table></span></div>
</div>