Page MenuHomePhabricator

Make the LTO comdat api more symbol table friendly
ClosedPublic

Authored by rafael on Oct 17 2016, 2:06 PM.

Details

Summary

In an IR symbol table I would expect the comdats to be represented as:

  • A table of strings, one for each comdat name.
  • Each symbol has an optional index into that table.

The natural api for accessing that would be

InputFile:
ArrayRef<StringRef> getComdats() const;

Symbol:
int getComdat() const;

This patch implements an API as close to that as possible. The implementation on top of the current IRObjectFile is a bit hackish, but should map just fine over a symbol table and is very convenient to use (I will upload the lld patch).

Diff Detail

Event Timeline

rafael updated this revision to Diff 74900.Oct 17 2016, 2:06 PM
rafael retitled this revision from to Make the LTO comdat api more symbol table friendly.
rafael updated this object.
rafael added reviewers: mehdi_amini, pcc, davide.
rafael added a subscriber: llvm-commits.
mehdi_amini edited edge metadata.Oct 17 2016, 2:09 PM

While the symbols in the "on disk" representation would have an int pointing to a table, it is not clear to me why you couldn't return directly a StringRef from the symbol?

davide added inline comments.Oct 17 2016, 7:50 PM
lib/LTO/LTO.cpp
231

Can you please document this API?
e.g. we return if the symbol has no GlobalValue associated etc..

davide edited edge metadata.Oct 17 2016, 7:53 PM

Ideally all the APIs in this file should be documented (at least the non-trivial ones).
We don't return a comdat pointer but a comdat handle/identifier, so, maybe the API should be renamed accordingly to getComdatIndex/Identifier or something?

rafael updated this revision to Diff 75487.Oct 21 2016, 2:34 PM
rafael edited edge metadata.

Use better names and add comments.

Thank you for taking care of my comments. I'm happy with it. Mehdi, what do you think?

include/llvm/LTO/LTO.h
185–188

Uh, this is not ideal. Hopefully we can get rid of it once we migrate to a bitcode symtab.

There are no changes in llvm-lto2, that makes me worried on the coverage of these code path.

include/llvm/LTO/LTO.h
188

The FIXME is not very clear "the way aliases are implemented" -> I have no idea what this is about.

mehdi_amini added inline comments.Oct 22 2016, 11:27 AM
lib/LTO/LTO.cpp
239

The error looks like the caller is not supposed to call this API under some circumstances, but it is not clear to me how it is supposed to figure, or recover.

mehdi_amini accepted this revision.Oct 22 2016, 11:30 AM
mehdi_amini edited edge metadata.

I didn't look at the state before, most of this is mechanically translated.

The fact that llvm-lto2 is not testing this API is still an issue, but shouldn't block this.

If you can improve the FIXME to better describe the error situation that'd be great.

LGTM.

This revision is now accepted and ready to land.Oct 22 2016, 11:30 AM
davide closed this revision.Nov 1 2016, 11:01 AM

Was this committed? If yes can you add the revision here?

(From Rafael on the mailing list) r285061