This is an archive of the discontinued LLVM Phabricator instance.

Move the section name from GlobalObject to the LLVMContext
ClosedPublic

Authored by rnk on Dec 28 2016, 6:05 PM.

Details

Summary

Convention wisdom says that bytes in Function are precious, and the
vast, vast majority of globals do not live in special sections. Even
when they do, they tend to live in the same section. Store the section
name on the LLVMContext in a StringSet, and maintain a map from
GlobalObject* to section name like we do for metadata, prefix data, etc.

The fact that we've survived this long wasting at least three pointers
of space in Function suggests that Function bytes are perhaps not as
precious as we once thought. Given that most functions have metadata
attachments when debug info is enabled, we might consider adding a
pointer here to make that access more efficient.

Event Timeline

rnk updated this revision to Diff 82640.Dec 28 2016, 6:05 PM
rnk retitled this revision from to Move the section name from GlobalObject to the LLVMContext.
rnk updated this object.
rnk added reviewers: jlebar, dexonsmith.
rnk added a subscriber: llvm-commits.
mehdi_amini accepted this revision.Dec 28 2016, 6:28 PM
mehdi_amini added a reviewer: mehdi_amini.
mehdi_amini added a subscriber: mehdi_amini.

LGTM.

include/llvm/IR/GlobalObject.h
41

We could do the same for the comdat?

75

Private?

lib/IR/Globals.cpp
188

I'd write it along these lines:

unsigned Mask = 1 << HasSectionHashEntryBit;
auto Bitfield = getGlobalValueSubClassData();
if (HasSection) 
  // Set the `HasSectionHashEntryBit`
  Bitfield |= Mask;
else
  // Clear the `HasSectionHashEntryBit`
  Bitfield &= ~Mask;
setGlobalValueSubClassData(Bitfield);

Seems less complex to read to me than the | with a ternary on the RHS that may lead to a 0.

Otherwise extracting it in a setGlobalValueSubClassDataBitValue(unsigned BitNo, bool Value) would make the implementation of it trivial.

This revision is now accepted and ready to land.Dec 28 2016, 6:28 PM
mehdi_amini added inline comments.Dec 28 2016, 6:29 PM
include/llvm/IR/GlobalObject.h
72

I'd comment here that the access is not efficient and client is advise to check for hasSection() before calling it.

rnk marked 2 inline comments as done.Dec 28 2016, 6:47 PM
rnk added inline comments.
include/llvm/IR/GlobalObject.h
41

I'd want to do performance measurements before doing that. For C++ targeting not MachO, basically everything has a comdat, and I don't want to regress that. I'm aware that nothing has a comdat on MachO, but it's something we'd want to measure before changing.

Also, the wins are much smaller. std::string is usually 3 pointers, vs one for comdat.

72

Done, added more Doxygen.

lib/IR/Globals.cpp
188

Yeah, setGlobalValueSubClassDataBitValue sounds like the right thing.

rnk updated this revision to Diff 82642.Dec 28 2016, 6:48 PM
rnk marked an inline comment as done.
rnk edited edge metadata.
  • Implement Mehdi's suggestions
mehdi_amini added inline comments.Dec 28 2016, 7:12 PM
include/llvm/IR/GlobalObject.h
41

Makes sense, my thought was that it is the kind of information that is not "frequently" queried, so it should be fine, but it may not be true, measuring is the right thing to do!

jlebar added inline comments.Dec 29 2016, 9:43 AM
lib/IR/Globals.cpp
180

Don't we "leak" section strings now? (That is, if I call setSection(x) a bunch of times on the same object, all of the unique values of x will persist in the context forever.)

mehdi_amini added inline comments.Dec 29 2016, 10:06 AM
lib/IR/Globals.cpp
180

Yes: this is the common practice with the LLVMContext (Metadata, Type, Constant, ...)

jlebar accepted this revision.Dec 29 2016, 10:30 AM
jlebar edited edge metadata.
jlebar added inline comments.
lib/IR/Globals.cpp
180

Ah, okay then!

rnk added inline comments.Dec 29 2016, 10:56 AM
lib/IR/Globals.cpp
180

Didn't we used to have some kind of GC-like functionality where we try to delete dead constants? dropTriviallyDeadConstantArrays looks a little specific to arrays. Constant aggregate initializers in particular accumulate pretty quickly. If such an API existed, it would be easy to GC the strings by building a new set by iterating the GlobalObjectSections map, updating the references there, and swapping that into place on the context.

This revision was automatically updated to reflect the committed changes.