This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail


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.

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.

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
749 ↗(On Diff #61332)

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

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


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
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.


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
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
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.