Page MenuHomePhabricator

Move legacy LTO interface headers to legacy/ directory.
ClosedPublic

Authored by pcc on Jul 8 2016, 2:44 PM.

Details

Summary

This library will be the home of a new resolution-based LTO interface (D20268),
which should not be confused with the existing legacy LTO interface in this
directory.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 63321.Jul 8 2016, 2:44 PM
pcc retitled this revision from to Move LTO.cpp to a new LTOResolution library..
pcc updated this object.
pcc added reviewers: tejohnson, mehdi_amini.
pcc added a subscriber: llvm-commits.
mehdi_amini edited edge metadata.Jul 8 2016, 4:08 PM

Can you mention what is a "resolution-based LTO interface" and why do we need a separate library?
(Or points to another revision if it was discussed somewhere else)

pcc added a comment.Jul 8 2016, 4:09 PM

This is D20268, I'll copy some of the info from there.

OK, having a separate library needs some motivation, I think this has be diluted in the original discussion so I lost track of it.
The name "Resolution" made me think that it'll be a sub-part of the LTO component of LLVM dealing *only* with symbol-resolution.

pcc updated this object.Jul 8 2016, 4:41 PM
pcc edited edge metadata.
pcc added a comment.Jul 8 2016, 4:50 PM

Okay, I've updated the summary to include the motivation.

This seems very "artificial" to me and I'm not convince a new library is justified right now.

In include/llvm/LTO/ I can see: LTOCodeGenerator.h LTOModule.h ThinLTOCodeGenerator.h

What's the plan for all of that? If we're just dealing with legacy here, I'd just move all these into include/llvm/LTO/legacy/ instead. There's not much harm to have the three implementation file in lib/LTO.
At some point if libLTO.dylib is the only remaining user for these interface and the implementation targets the new LTO API, they could be moved to tools/lto/* and the header made private there.

pcc added a comment.Jul 8 2016, 5:07 PM

If we're just dealing with legacy here, I'd just move all these into include/llvm/LTO/legacy/ instead.

I think that would work as well, but I wanted to make a clean break.

We'll also have LTOBackend.cpp which will be used by clang. I think Teresa mentioned some problems depending on the existing LTO library from clang, but let me see if I can get it to work. If not, I guess we can move the implementation to lib/LTO/Legacy.

In D22173#479018, @pcc wrote:

If we're just dealing with legacy here, I'd just move all these into include/llvm/LTO/legacy/ instead.

I think that would work as well, but I wanted to make a clean break.

We'll also have LTOBackend.cpp which will be used by clang. I think Teresa mentioned some problems depending on the existing LTO library from clang

Teresa, can you refresh us on this aspect?

tejohnson edited edge metadata.Jul 8 2016, 5:27 PM
tejohnson added a subscriber: tejohnson.

I don't remember if it was theoretical or if I tried this and ran into
issues. Thinking about it some more now - I suppose adding new dependence
from clang to llvm should never introduce a circular dependence? So I don't
know what I was concerned about...

pcc updated this revision to Diff 63893.Jul 13 2016, 5:59 PM
pcc edited edge metadata.
  • Move legacy LTO interface headers to legacy/ directory
pcc added a comment.Jul 13 2016, 6:00 PM

A local change which replaced the dependency on LTOResolution with LTO seems to work, so I've done as Mehdi suggested.

pcc retitled this revision from Move LTO.cpp to a new LTOResolution library. to Move legacy LTO interface headers to legacy/ directory..Jul 13 2016, 6:01 PM
pcc updated this object.
mehdi_amini accepted this revision.Jul 14 2016, 1:57 PM
mehdi_amini edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jul 14 2016, 1:57 PM
This revision was automatically updated to reflect the committed changes.