This is an archive of the discontinued LLVM Phabricator instance.

CodeGen: Replace ThinLTO backend implementation with a client of LTO/Resolution.
ClosedPublic

Authored by pcc on Jun 20 2016, 6:23 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 61332.Jun 20 2016, 6:23 PM
pcc retitled this revision from to CodeGen: Replace ThinLTO backend implementation with a client of LTO/Resolution..
pcc updated this object.
pcc added reviewers: tejohnson, mehdi_amini.
pcc added a subscriber: cfe-commits.
tejohnson edited edge metadata.Jun 27 2016, 10:19 AM

Sorry for the delay, comments below.

lib/CodeGen/BackendUtil.cpp
733 ↗(On Diff #61332)

Use ComputeCrossModuleImportForModule instead, since this is only for a single module and we don't need the ExportLists (and is the same routine invoked currently by the FunctionImportPass).

749 ↗(On Diff #61332)

OwnedImports is never read from.

lib/Driver/Tools.cpp
3894 ↗(On Diff #61332)

This change and the associated test change seem independent of the rest of the patch and could be submitted now independently.

pcc updated this revision to Diff 62160.Jun 28 2016, 6:05 PM
pcc marked 2 inline comments as done.
pcc edited edge metadata.
  • Use ComputeCrossModuleImportForModule
lib/CodeGen/BackendUtil.cpp
749 ↗(On Diff #61332)

Yes, but it is needed to preserve ownership of the module's memory buffer until we exit the function scope.

lib/Driver/Tools.cpp
3894 ↗(On Diff #61332)
tejohnson accepted this revision.Jun 28 2016, 7:20 PM
tejohnson edited edge metadata.

LGTM

lib/CodeGen/BackendUtil.cpp
757 ↗(On Diff #62160)

Ok, please add a comment.

This revision is now accepted and ready to land.Jun 28 2016, 7:20 PM
tejohnson added inline comments.Jul 1 2016, 7:52 AM
lib/CodeGen/BackendUtil.cpp
758 ↗(On Diff #62160)

Can we pass along -save-temps here (i.e. invoke Conf.addSaveTemps)?

pcc updated this revision to Diff 63908.Jul 13 2016, 8:10 PM
pcc edited edge metadata.

Refresh

Note that this will have the side effect of enabling linkonce/weak resolution and internalization for the distributed backends.

I had just sent a separate patch (D22356) to do the former, which includes a fix for a subtle linking issue in the distributed backend case. I was planning to follow that up with another patch to add the internalization, after first removing the forced linkonce linking. However, I like having the weak resolution and internalization outside of the FunctionImporter pass, as is done here. So I think the best thing is to change my D22356 to not add weak resolution to the pass, but just have it address the linker issue requiring the new PreserveNonPrevailing flag and the test case. It should probably go in immediately after this one (or my distributed backend testing will start getting failures).

mehdi_amini added inline comments.Jul 14 2016, 12:13 PM
lib/CodeGen/BackendUtil.cpp
740 ↗(On Diff #63908)

This should go away at some point right?
Just to double check if my memory is correct: the linker plugin will emit the import decision and the backend will take it as an input instead of recomputing anything?

tejohnson added inline comments.Jul 14 2016, 12:35 PM
lib/CodeGen/BackendUtil.cpp
740 ↗(On Diff #63908)

The way they are passed down is via the individual combined index. So it simply computes via what is in the index, which will only include summaries for what should be imported. I suppose we could make the logic even simpler tho and just blindly import what is in the index.

Peter, can you add a fixme?

pcc updated this revision to Diff 64043.Jul 14 2016, 2:16 PM
  • Add a FIXME
pcc marked an inline comment as done.Jul 14 2016, 2:17 PM
mehdi_amini accepted this revision.Jul 14 2016, 2:18 PM
mehdi_amini edited edge metadata.

(I didn't mark it as accepted because Teresa did, but in case you're waiting for me, don't)

This revision was automatically updated to reflect the committed changes.