Page MenuHomePhabricator

Rework linkInModule(), making it oblivious to ThinLTO
ClosedPublic

Authored by mehdi_amini on Mar 12 2016, 2:47 PM.

Details

Summary

ThinLTO is relying on linkInModule to import selected function.
However a lot of "magic" was hidden in linkInModule and the IRMover,
who would rename and promote global variables on the fly.

This is moving to an approach where the steps are decoupled and the
client is reponsible to specify the list of globals to import.
As a consequence some test are changed because they were relying on
the previous behavior which was importing the definition of *every*
single global without control on the client side.
Now the burden is on the client to decide if a global has to be imported
or not.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Rework linkInModule(), making it oblivious to ThinLTO.
mehdi_amini updated this object.
mehdi_amini added a reviewer: tejohnson.
mehdi_amini added a subscriber: llvm-commits.
tejohnson edited edge metadata.Mar 14 2016, 6:28 AM

This is a nice cleanup and less invasive than I feared. I am concerned as noted below about losing all variable definition importing. Will try to apply this and do a performance comparison with spec to see if there are currently any effects.

lib/Linker/LinkModules.cpp
404–405

This seems like it should be redundant/combined with the condition added to the below if statement. Can't the below if statement just be "if (!isPerformingImport() && !DGV && ...)"? I.e. not need to check GlobalsToImport again since it was already checked in doImportAsDefinition, and if we reach the below if statement when isPerformingImport() then we do want this symbol?

Alternatively, you could combine this and the below if into one block like:

if (isPerformingImport()) {

if (!doImportAsDefinition(&GV))
    return false;

} else if (!DGV && ...)

return false;
lib/Transforms/IPO/FunctionImport.cpp
287

Moving this is unrelated to the rest of the patch?

355

Rename FunctionsToImport to GlobalsToImport here as well?

Although currently with this change it looks like we will never import a global variable definition since the importer is only adding functions to this list, right? I wonder if there is a performance loss. Will apply this and try with spec.

test/Transforms/FunctionImport/Inputs/funcimport.ll
23 ↗(On Diff #50528)

Why was globalfunc3 and staticvar2 added? Don't see anything checking them in the test.

tools/llvm-link/llvm-link.cpp
211

Extend and invoke llvm::renameModuleForThinLTO instead. Ditto for similar block added to FunctionImport.cpp.

280

Use llvm::renameModuleForThinLTO instead

mehdi_amini edited edge metadata.

Update to take Teresa's comments into account.

mehdi_amini marked 4 inline comments as done.

Update tests as well

lib/Transforms/IPO/FunctionImport.cpp
355

Yes no import of globals yet, we operate only on functions. It makes sense only for constant, right? I don't know if we have this information in the summary?

test/Transforms/FunctionImport/Inputs/funcimport.ll
23 ↗(On Diff #50528)

This is a partial leftover from further changes that haven't been submitted yet (testing that we promote only what is imported elsewhere). Will remove from this patch.

tejohnson added inline comments.Mar 16 2016, 4:18 AM
lib/Transforms/IPO/FunctionImport.cpp
355

Ran spec with this patch applied, no significant performance delta, so this is not currently a big issue.

Might have to augment the summary again to note if they are constant.

442

Use a default on the new parameter of nullptr instead of passing it explicitly?

lib/Transforms/Utils/FunctionImportUtils.cpp
226

Shouldn't the getNewExportedValues be added to GlobalsToImport if non-null?

tools/llvm-link/llvm-link.cpp
290–291

I thought I removed the PreserveModules code from HEAD awhile back? How did it get merged back?

I really like this change, but Teresa should be the one to give the final LGTM.

include/llvm/Linker/Linker.h
49

Can you remove the llvm/IR/ModuleSummaryIndex.h include?

Rebase on top of trunk. And remove the include pointed by Rafael.

mehdi_amini marked 2 inline comments as done.

Add a default value for renameModuleForThinLTO parameter.

mehdi_amini marked 3 inline comments as done.Mar 16 2016, 9:49 AM

Rebase on trunk

tejohnson added inline comments.Mar 16 2016, 2:49 PM
lib/Transforms/Utils/FunctionImportUtils.cpp
226

Everything looks good except that this one still needs to be addressed.

Sorry missed your earlier comment...
We can totally remove "NewExportedValues" as the responsibility is now
on the caller to specify the full list of symbols to promote.

mehdi_amini marked 2 inline comments as done.Mar 16 2016, 3:05 PM
mehdi_amini marked 2 inline comments as done.Mar 16 2016, 3:05 PM
tejohnson accepted this revision.Mar 16 2016, 4:04 PM
tejohnson edited edge metadata.
In D18122#376888, @joker.eph wrote:

Sorry missed your earlier comment...
We can totally remove "NewExportedValues" as the responsibility is now
on the caller to specify the full list of symbols to promote.

Hmm yes, I think this is left over from when we did promotion on the exporting side via the module linker.

Thanks for the cleanup. Lgtm with a couple of nits.

tools/llvm-link/llvm-link.cpp
196–199

Index declaration can be sunk inside if body now

268–269

This can be moved inside of if statement here too.

This revision is now accepted and ready to land.Mar 16 2016, 4:04 PM
mehdi_amini marked 2 inline comments as done.
mehdi_amini edited edge metadata.

Update before commit

Thanks!
Committed r263863

mehdi_amini closed this revision.Mar 18 2016, 5:45 PM