Page MenuHomePhabricator

[LTO] Basic support for internalize.
ClosedPublic

Authored by davide on Mar 23 2016, 2:26 PM.

Details

Summary

So, this is still a WIP but I'd like to discuss it to be sure we handle all the cases correctly.

  • Similarly to what the gold plugin does, every time we see a new bitcode file we collect the set of the used global variables and for each symbol defined in the bitcode file make a decision to internalize it or not.
  • The resolver tells us if a symbol is referenced from outside (this is pretty similar to what the gold linker does during symbol resolution, but they also take in account if a symbol is visible outside, which I plan to implement later). Note, I'm not sure the check added which check the flag is completely correct.
  • Once we are done with this step, *in* the combined object, we actually perform the internalize step changing linkage for each GV previously collected.

Some comments:
In gold, the linker tells back to the plugin what's the resolution for a given symbol. WRT internalize, two resolutions are relevant:
-> PREVAILING_DEF_IRONLY (this is the prevailing definition of the symbol, with no references from regular objects)
-> PREVAILING_DEF_IRONLY_EXP (as above, but the symbol is exported to the dynamic symbol table)

We currently cover only the first case properly.

Diff Detail

Repository
rL LLVM

Event Timeline

davide updated this revision to Diff 51471.Mar 23 2016, 2:26 PM
davide retitled this revision from to [LTO] WIP: Internalize.
davide updated this object.
davide set the repository for this revision to rL LLVM.
davide added a subscriber: llvm-commits.
rafael added inline comments.Mar 23 2016, 2:35 PM
ELF/LTO.cpp
82 ↗(On Diff #51471)

You can leave this for a future patch, no ?

ELF/Symbols.cpp
19 ↗(On Diff #51471)

dead include.

ELF/Symbols.h
185 ↗(On Diff #51471)

Would it be possible to reuse IsUsedInRegularObj for this?

davide updated this revision to Diff 51488.Mar 23 2016, 4:03 PM
davide removed rL LLVM as the repository for this revision.
davide retitled this revision from [LTO] WIP: Internalize to [LTO] Basic support for internalize..
mehdi_amini added inline comments.Mar 23 2016, 4:40 PM
ELF/LTO.cpp
107 ↗(On Diff #51488)

Can't you change the linkage here immediately instead of using s StringSet.

As a side note, I usually not look at lld but in general this code is lacking documentation, if it was in LLVM it would require a lot more.

ruiu added a subscriber: ruiu.Mar 24 2016, 1:43 AM
ruiu added inline comments.
ELF/LTO.cpp
104 ↗(On Diff #51488)

I don't want to sprinkle FIXMEs to new patches because it wouldn't help readers but instead asking a question to readers. If you are not sure what the correct thing to do but decided to go with some code, then please justify your decision in comment, like

// We are not sure what to do if we are creating a shared object file.
// For now, we do blah blah because blah blah.
127–131 ↗(On Diff #51488)
for (const auto &Name : Internalize)
  if (GlobalValue *GV = ...)
    internalize(*GV);

Also, please do not add a blank line at beginning of a function.

ELF/LTO.h
48 ↗(On Diff #51488)

StringSet copies strings to its internal buffer. Is this what you want?

"Internalize" is a verb. I think you want to name it a noun.

ELF/SymbolTable.cpp
217 ↗(On Diff #51488)

This comment needs to be more descriptive for those who don't understand what is going on in LTO.

davide updated this revision to Diff 51576.Mar 24 2016, 11:36 AM

Addressed comments.

rafael edited edge metadata.Mar 24 2016, 12:18 PM

This looks a lot better.

Testcase? :-)

ELF/LTO.cpp
18 ↗(On Diff #51576)

Dead include?

82 ↗(On Diff #51576)

This can also go in a future patch, no?

113 ↗(On Diff #51576)

ExportDynamic should have the same treatment as shared. Not sure if we already have a convenient predicate for either.

126 ↗(On Diff #51576)

You only add symbols that are not internal, I think you can assert this.

136 ↗(On Diff #51576)

You can assert GV for now I think.

ELF/SymbolTable.cpp
221 ↗(On Diff #51576)

I am not sure just Undefined is sufficient.

If a weak symbol in an object file is replaced with a strong symbol in a .bc file, there can be a reference from the strong symbol to the bit code one.

davide marked 4 inline comments as done.Mar 24 2016, 1:10 PM

Oh, the testcase was there before (just didn't show up in my last rev, weird).
Addressing some of your comments + reply.

ELF/LTO.cpp
113 ↗(On Diff #51576)

Can I do this separately? (And add a specific test for it)

ELF/SymbolTable.cpp
221 ↗(On Diff #51576)

Can I do this separately?

davide updated this revision to Diff 51589.Mar 24 2016, 1:10 PM
davide edited edge metadata.
davide added inline comments.Mar 24 2016, 6:22 PM
ELF/SymbolTable.cpp
221 ↗(On Diff #51589)

Can we use isUsedInRegularObj or do you prefer me to add a new flag isInRealELF?

silvas added a subscriber: silvas.Mar 24 2016, 8:33 PM
silvas added inline comments.
ELF/SymbolTable.cpp
221 ↗(On Diff #51589)

DefinedElf and UndefinedElf already capture that distinction I think. I don't think a separate flag is necessary.

grimar added a subscriber: grimar.Mar 25 2016, 3:33 AM
grimar added inline comments.
test/ELF/lto/internalize-basic.ll
4 ↗(On Diff #51589)

"-o -" ? That looks a bit wierd to have "-" as a file name IMO, may be just standart temporary file would be better here ?

davide added inline comments.Mar 25 2016, 11:04 AM
test/ELF/lto/internalize-basic.ll
4 ↗(On Diff #51589)

It's stdout.

silvas added inline comments.Mar 25 2016, 1:30 PM
test/ELF/lto/internalize-basic.ll
4 ↗(On Diff #51589)

Doesn't llvm-dis default to stdout? I don't think -o - os needed.

Please extend the test to coven a symbol that is *not* internalized.

ELF/LTO.cpp
79 ↗(On Diff #51589)

This is now unused.

ELF/SymbolTable.cpp
225 ↗(On Diff #51589)

So, do you need to set this in here at all?

We already have

if (IsUsedInRegularObj || Other->IsUsedInRegularObj)
  IsUsedInRegularObj = Other->IsUsedInRegularObj = true;
test/ELF/lto/internalize-basic.ll
4 ↗(On Diff #51589)

It is not needed if you use it as

llvm-dis < %t2.lto.bc | FileCheck

Which is probably a bit better as llvm-dis doesn't get the filename.

17 ↗(On Diff #51589)

What about _start?

davide updated this revision to Diff 51763.Mar 27 2016, 6:20 PM

Address Rafael's comments. Thanks for the review.

davide updated this revision to Diff 51765.Mar 27 2016, 6:22 PM

Remove another dead include.

rafael added inline comments.Mar 27 2016, 6:55 PM
ELF/Symbols.h
95 ↗(On Diff #51765)

You don't need to change this now.

davide updated this revision to Diff 51767.Mar 27 2016, 7:03 PM

Remove other dead code.

rafael accepted this revision.Mar 28 2016, 4:08 AM
rafael edited edge metadata.

LGTM

This revision is now accepted and ready to land.Mar 28 2016, 4:08 AM
This revision was automatically updated to reflect the committed changes.