This is an archive of the discontinued LLVM Phabricator instance.

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

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.
include/llvm/LTO/legacy/LTOModule.h