This is an archive of the discontinued LLVM Phabricator instance.

Add a FunctionImporter helper to perform summary-based cross-module function importing
ClosedPublic

Authored by mehdi_amini on Nov 22 2015, 9:39 PM.

Details

Summary

This is a helper to perform cross-module import for ThinLTO. Right now
it is importing naively every possible called functions.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Add a FunctionImporter helper to perform summary-based cross-module function importing.
mehdi_amini updated this object.
mehdi_amini added a reviewer: tejohnson.
mehdi_amini added subscribers: llvm-commits, dexonsmith.
tejohnson edited edge metadata.Nov 23 2015, 7:34 AM

Hi Mehdi,

Didn't realize you were working on this, I was readying a slightly different version. We can work from your version though. Some initial comments below.

There are a couple of pieces missing. One thing that is missing here is to import for calls from imported functions, noted below. Another thing is that there potentially needs to be static promotion/renaming in the current module being compiled. I have a clang FE patch to handle that part, since it should be done early.

lib/Transforms/IPO/FunctionImport.cpp
51

I was not planning to keep all the modules being imported from in memory, to minimize the memory overhead. That does require reparsing the initial part of the bitcode, but that is quite fast in practice.

The metadata importing patch handles this by not parsing/materializing metadata until a separate metadata parse/link step. Note that even if we are to save the modules as in this approach we would need to delay the metadata parsing/linking until the end of all importing to reduce memory overhead, so much of that patch is still needed.

82

I think this should save the called function pointers in a set, rather than the name. You also don't need the DefinedFunctions set. Just check here if the called function is a declaration, and don't add to the called function set if not.

96

Not needed as noted above.

106

We should always have the function info populated, even if we are doing lazy parsing of summaries. The case where we decided not to include a function in the index (because we decided earlier that it was not eligible for or profitable to import) should be handled by the above check on the returned iterator.

112

With my comdat importing patch we probably shouldn't even hit this situation as I made a change to import referenced comdat groups. Fine to pick the first entry here for now though, we can reevaluate later.

120

Add a note that we should eventually check if we are doing lazy parsing of summaries, in which case we import lazily at this point.

145

Need to get the function for the alias base object. Here's the code from my version of the importer, you can probably just drop this in and modify the variable names:

GlobalValue *SGV = SrcM->getNamedValue(FunctionName);
Function *SF = dyn_cast<Function>(SGV);
if (!SF && isa<GlobalAlias>(SGV)) {
  auto *SGA = dyn_cast<GlobalAlias>(SGV);
  SF = dyn_cast<Function>(SGA->getBaseObject());
}
160

After this, we need to walk the imported function copy in the destination and find new callsites that need to be imported. For now if you want to add a FIXME comment to that effect it is fine and I can add that along with profitability heuristics. Otherwise you will end up importing all functions in the call tree below functions in this module...

[...] Another thing is that there potentially needs to be static promotion/renaming in the current module being compiled. I have a clang FE patch to handle that part, since it should be done early.

I guess you won't be doing this through clang since you are planning to invoke from ld64, but remember to do this part there for each module being compiled. Will need to do from 'opt' as well for testing. My clang patch also includes support for an option to trigger the pass and pass down the index file path via a CodeGenOption, with corresponding support in llvm to invoke the pass. I can add that part after this goes in.

Hi Teresa,

Thanks for the review, I should have added in the description that I was mostly interested to share some high level design and APIs as I am working on ld64 integration. I know you're also working on the same functionality, I just wrote this one to be able to test cross-module importing without heuristic (i.e. import one level of the call graph unconditionally).

So I don't mind at all dropping this patch if you already have a replacement (I'll continue to use this code for my prototype till you're ready), or if this patch is close enough to what you're working on we can integrate this and work iteratively to improve it. As you prefer :)

Good point that the renaming is still missing, this is not gonna be trivial in the plugin I think as we will need to communicate the renaming and the new edges between file to the linker. Doing it in the FE would be nice, but I guess it won't be possible as it requires to have the index.
This is why I'm glad I posted this as it already exposes some interface with the linker I'm missing at that time.

Thanks,

Mehdi

lib/Transforms/IPO/FunctionImport.cpp
51

Note that these are only the "Lazy" modules used for importing. They should consume less memory since they don't initialize the bodies.
That said, I agree that some measurements need to be done to explore the cost tradeoff.

82

Thanks good point.

106

IIUC the answer is "yes we could assert here", right?

160

Yes I didn't want to import the full call-graph, so I limited arbitrarily to one level.
I guess we'll need to move to a worklist pattern for this loop and enqueue imported function?

In D14914#295184, @joker.eph wrote:

Hi Teresa,

Thanks for the review, I should have added in the description that I was mostly interested to share some high level design and APIs as I am working on ld64 integration. I know you're also working on the same functionality, I just wrote this one to be able to test cross-module importing without heuristic (i.e. import one level of the call graph unconditionally).

So I don't mind at all dropping this patch if you already have a replacement (I'll continue to use this code for my prototype till you're ready), or if this patch is close enough to what you're working on we can integrate this and work iteratively to improve it. As you prefer :)

No problem, this is only a piece of the stuff I was working on (have the clang part as well), and is a bit simpler since I was trying to incorporate some initial profitability checking and iteratively handling the imported calls. I am thinking it is good to get this one in with the current functionality to serve as the general framework and I can add that part shortly. That way we can start iterating and refining this more quickly.

Good point that the renaming is still missing, this is not gonna be trivial in the plugin I think as we will need to communicate the renaming and the new edges between file to the linker. Doing it in the FE would be nice, but I guess it won't be possible as it requires to have the index.

I am passing in the index to clang but this is to support distributed builds where we are invoking the backend piece as a separate job, so that won't help your use case. I will need to do something similar to what you are dealing with in the gold-plugin for supporting single machine ThinLTO builds too. 

This is why I'm glad I posted this as it already exposes some interface with the linker I'm missing at that time.

Thanks,

Mehdi

lib/Transforms/IPO/FunctionImport.cpp
106

Yep, sorry, I meant you can assert here.

160

Yep, I think that is what is going to be needed (e.g. some kind of priority queue), but go ahead and leave as-is for now, that can be added when the functionality for adding imported callsites is added.

tejohnson added inline comments.Nov 23 2015, 11:23 AM
lib/Transforms/IPO/FunctionImport.cpp
185

Remove and replace call with new llvm:getFunctionIndexForFile routine just added in r253903.

mehdi_amini edited edge metadata.

Update taking comment in to account (hopefully all...)

LGTM with one issue

lib/Transforms/IPO/FunctionImport.cpp
92

I just hit an issue with the earlier version of this patch I had downloaded and modified to store a worklist of Function pointers instead, and a slightly modified test case. The module linker may replace the dest module Function (erasing it from the parent) if it creates a new GV while linking global value protos. It might do this for other declarations it finds in the import module besides the import function. I had forgotten about this, I was dealing with it in my version of the import pass by passing in the structure to update with the new Function. But that is probably overkill. So unfortunately I think we may need to go back to tracking the names of function decls instead of the Function pointers.

mehdi_amini added inline comments.Nov 23 2015, 5:29 PM
lib/Transforms/IPO/FunctionImport.cpp
51

About caching the Module, the question is also if I have a call to foo() and bar() in module Y, both function being defined in module Y, this allows to transparently load Y only once when importing into Y.

92

Do you mean that while importing function foo() into module A, the Linker can change a declaration for function bar(), invalidating the Function pointer to bar() that may be in the worklist?

tejohnson added inline comments.Nov 23 2015, 7:32 PM
lib/Transforms/IPO/FunctionImport.cpp
52

Sorry, is Y the module we are importing from or to in your example? If we are importing into it, we only load it once. If we are importing foo() and bar() from it, then yes, without caching the modules in between imports Y needs to be loaded twice, but only one function is parsed/materialized on each load. It is a time vs memory tradeoff that we'll have to evaluate.

Let me know if I didn't understand your scenario though.

92

Yes. For example, in ModuleLinker::shouldLinkFromSource we will link from source (creating a new GV in the process with potentially new linkage type) when the dest is a declaration. We will even do this in certain cases when the source is only a declaration (weak linkage). So for example we create a new GV and delete the old dest GV for weakalias in your test case, which is an alias but is imported as a function decl since we aren't importing its aliasee. With the test case as-is I think we get lucky, but when I modified it I hit a seg fault somewhere because the Function in the work list is used after freed (apparently just luck that the memory was not yet overwritten).

Back to a string set

mehdi_amini added inline comments.Nov 23 2015, 9:18 PM
lib/Transforms/IPO/FunctionImport.cpp
52

I think we're on the same page here :)

tejohnson accepted this revision.Nov 23 2015, 9:41 PM
tejohnson edited edge metadata.

LGTM. Thanks.

This revision is now accepted and ready to land.Nov 23 2015, 9:41 PM