Page MenuHomePhabricator

FunctionImport: Use IRMover directly. NFCI.
ClosedPublic

Authored by pcc on Feb 2 2017, 12:05 PM.

Details

Summary

The importer was previously using ModuleLinker in a sort of "IRMover mode". Use
IRMover directly instead in order to remove a level of indirection.

I will remove all importing support from ModuleLinker in a separate
change.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Feb 2 2017, 12:05 PM
tejohnson added inline comments.Feb 2 2017, 1:17 PM
llvm/lib/Transforms/IPO/FunctionImport.cpp
724 ↗(On Diff #86868)

I guess you have to do this to make it NFC, since it looks like we were doing this in the ModuleLinker.
Interesting...I didn't realize or had forgotten that the ModuleLinker would do this - so essentially we always import an alias if its aliasee is linkonce_odr and marked for import? That seems like something we want to get rid of (can be in a follow-up change so this remains NFC). We've tried to get to the point where we only import values decided on by the thin link. This is presumably a holdover from the old scheme of always importing linkonce_odr references...

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
28 ↗(On Diff #86868)

This looks like a bug fix?

pcc updated this revision to Diff 86905.Feb 2 2017, 3:24 PM
  • Add comment
llvm/lib/Transforms/IPO/FunctionImport.cpp
724 ↗(On Diff #86868)

Yes, this was needed in order to be NFC. I've added a FIXME to remove.

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
28 ↗(On Diff #86868)

Yes, and also needed in order to be NFC. Without this we would import @linkonceODRfuncLinkonceAlias in test/ThinLTO/X86/alias_import.ll because the logic for whether to put globals in GlobalsToImport differed from that in FunctionImportGlobalProcessing::doImportAsDefinition and we would normally only import globals that satisfied both (this is because we were querying the latter on LinkModules.cpp:394 in the old code and querying the former on line 277).

But naturally that raises the question of why the change on line FunctionImport.cpp:724 is needed at all. The answer is that we only call shouldLinkFromSource if the destination defines a GV with the same name, so if the destination did not define the GV we would only test FunctionImportGlobalProcessing::doImportAsDefinition.

In short: ModuleLinker is complicated, and it's definitely a Good Thing that we're moving away from it.

mehdi_amini accepted this revision.Feb 2 2017, 4:32 PM

LGTM, but please wait for Teresa :)

llvm/lib/Transforms/IPO/FunctionImport.cpp
724 ↗(On Diff #86868)

I didn't realize or had forgotten that the ModuleLinker would do this

This is exactly what always bothered me with us using the ModuleLinker instead of the IRMover :)

This revision is now accepted and ready to land.Feb 2 2017, 4:32 PM
tejohnson accepted this revision.Feb 2 2017, 6:42 PM

LGTM but please remove the NFC from the description, due to the isInterposable change and the comdat handling difference noted on IRC. I did build SPEC with this change and it isn't causing any failures (there is a failure to link soplex due to hidden symbols, but it is still there without this patch and likely due to D28978).

This revision was automatically updated to reflect the committed changes.