Move the section name from GlobalObject to the LLVMContext

[rnk]

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.

[mehdi_amini]

LGTM.

include/llvm/IR/GlobalObject.h

41 ↗

(On Diff #82640)

We could do the same for the comdat?

75 ↗

(On Diff #82640)

Private?

lib/IR/Globals.cpp

188 ↗

(On Diff #82640)

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.

[mehdi_amini]

include/llvm/IR/GlobalObject.h

72 ↗

(On Diff #82640)

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

[rnk]

include/llvm/IR/GlobalObject.h

41 ↗

(On Diff #82640)

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 ↗

(On Diff #82640)

Done, added more Doxygen.

lib/IR/Globals.cpp

188 ↗

(On Diff #82640)

Yeah, setGlobalValueSubClassDataBitValue sounds like the right thing.

[rnk]

• Implement Mehdi's suggestions

[mehdi_amini]

include/llvm/IR/GlobalObject.h

41 ↗

(On Diff #82640)

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]

lib/IR/Globals.cpp

180 ↗

(On Diff #82642)

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]

lib/IR/Globals.cpp

180 ↗

(On Diff #82642)

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

[jlebar]

lib/IR/Globals.cpp

180 ↗

(On Diff #82642)

Ah, okay then!

[rnk]

lib/IR/Globals.cpp

180 ↗

(On Diff #82642)

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.

llvm/trunk/include/llvm/IR/GlobalObject.h

protected:
   GlobalObject(Type *Ty, ValueTy VTy, Use *Ops, unsigned NumOps,
                LinkageTypes Linkage, const Twine &Name,
                unsigned AddressSpace = 0)
       : GlobalValue(Ty, VTy, Ops, NumOps, Linkage, Name, AddressSpace),
         ObjComdat(nullptr) {
     setGlobalValueSubClassData(0);
   }
 
-  std::string Section;     // Section to emit this into, empty means default
   Comdat *ObjComdat;
   enum {
     LastAlignmentBit = 4,
     HasMetadataHashEntryBit,
+    HasSectionHashEntryBit,
 
     GlobalObjectBits,
   };
   static const unsigned GlobalObjectSubClassDataBits =
       GlobalValueSubClassDataBits - GlobalObjectBits;
 
 private:
   static const unsigned AlignmentBits = LastAlignmentBit + 1;
   static const unsigned AlignmentMask = (1 << AlignmentBits) - 1;
   static const unsigned GlobalObjectMask = (1 << GlobalObjectBits) - 1;
 
 public:
   GlobalObject(const GlobalObject &) = delete;
 
   unsigned getAlignment() const {
     unsigned Data = getGlobalValueSubClassData();
     unsigned AlignmentData = Data & AlignmentMask;
     return (1u << AlignmentData) >> 1;
   }
   void setAlignment(unsigned Align);
 
   unsigned getGlobalObjectSubClassData() const;
   void setGlobalObjectSubClassData(unsigned Val);
 
-  bool hasSection() const { return !getSection().empty(); }
-  StringRef getSection() const { return Section; }
+  /// Check if this global has a custom object file section.
+  ///
+  /// This is more efficient than calling getSection() and checking for an empty
+  /// string.
+  bool hasSection() const {
+    return getGlobalValueSubClassData() & (1 << HasSectionHashEntryBit);
+  }
+
+  /// Get the custom section of this global if it has one.
+  ///
+  /// If this global does not have a custom section, this will be empty and the
+  /// default object file section (.text, .data, etc) will be used.
+  StringRef getSection() const {
+    return hasSection() ? getSectionImpl() : StringRef();
+  }
+
+  /// Change the section for this global.
+  ///
+  /// Setting the section to the empty string tells LLVM to choose an
+  /// appropriate default object file section.
   void setSection(StringRef S);
 
   bool hasComdat() const { return getComdat() != nullptr; }
   const Comdat *getComdat() const { return ObjComdat; }
   Comdat *getComdat() { return ObjComdat; }
   void setComdat(Comdat *C) { ObjComdat = C; }
 
   /// Check if this has any metadata.
public:
   static inline bool classof(const Value *V) {
     return V->getValueID() == Value::FunctionVal ||
            V->getValueID() == Value::GlobalVariableVal;
   }
 
   void clearMetadata();
 
 private:
+  void setGlobalObjectFlag(unsigned Bit, bool Val) {
+    unsigned Mask = 1 << Bit;
+    setGlobalValueSubClassData((~Mask & getGlobalValueSubClassData()) |
+                               (Val ? Mask : 0u));
+  }
+
   bool hasMetadataHashEntry() const {
     return getGlobalValueSubClassData() & (1 << HasMetadataHashEntryBit);
   }
   void setHasMetadataHashEntry(bool HasEntry) {
-    unsigned Mask = 1 << HasMetadataHashEntryBit;
-    setGlobalValueSubClassData((~Mask & getGlobalValueSubClassData()) |
-                               (HasEntry ? Mask : 0u));
+    setGlobalObjectFlag(HasMetadataHashEntryBit, HasEntry);
   }
+
+  StringRef getSectionImpl() const;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_IR_GLOBALOBJECT_H

llvm/trunk/lib/IR/Globals.cpp

 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/GlobalAlias.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "LLVMContextImpl.h"
 using namespace llvm;
 
 //===----------------------------------------------------------------------===//
 //                            GlobalValue Class
 //===----------------------------------------------------------------------===//
 
 // GlobalValue should be a Constant, plus a type, a module, some flags, and an
 // intrinsic ID. Add an assert to prevent people from accidentally growing
 // GlobalValue while adding flags.
 static_assert(sizeof(GlobalValue) ==
                   sizeof(Constant) + 2 * sizeof(void *) + 2 * sizeof(unsigned),
               "unexpected GlobalValue size growth");
 
+// GlobalObject adds a comdat.
+static_assert(sizeof(GlobalObject) == sizeof(GlobalValue) + sizeof(void *),
+              "unexpected GlobalObject size growth");
+
 bool GlobalValue::isMaterializable() const {
   if (const Function *F = dyn_cast<Function>(this))
     return F->isMaterializable();
   return false;
 }
 Error GlobalValue::materialize() {
   return getParent()->materialize(this);
 }
if (auto *GA = dyn_cast<GlobalAlias>(this)) {
     return nullptr;
   }
   // ifunc and its resolver are separate things so don't use resolver comdat.
   if (isa<GlobalIFunc>(this))
     return nullptr;
   return cast<GlobalObject>(this)->getComdat();
 }
 
-void GlobalObject::setSection(StringRef S) {
-  Section = S;
+StringRef GlobalObject::getSectionImpl() const {
+  assert(hasSection());
+  return getContext().pImpl->GlobalObjectSections[this];
+}
 
-  // The C api requires this to be null terminated.
-  Section.c_str();
+void GlobalObject::setSection(StringRef S) {
+  // Do nothing if we're clearing the section and it is already empty.
+  if (!hasSection() && S.empty())
+    return;
+
+  // Get or create a stable section name string and put it in the table in the
+  // context.
+  S = getContext().pImpl->SectionStrings.insert(S).first->first();
+  getContext().pImpl->GlobalObjectSections[this] = S;
+
+  // Update the HasSectionHashEntryBit. Setting the section to the empty string
+  // means this global no longer has a section.
+  setGlobalObjectFlag(HasSectionHashEntryBit, !S.empty());
 }
 
 bool GlobalValue::isDeclaration() const {
   // Globals are definitions if they have an initializer.
   if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(this))
     return GV->getNumOperands() == 0;
 
   // Functions are definitions if they have a body.

llvm/trunk/lib/IR/LLVMContextImpl.h

 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Dwarf.h"
 #include "llvm/Support/YAMLTraits.h"
#include "llvm/IR/Metadata.def"
   StringMap<unsigned> CustomMDKindNames;
 
   /// Collection of per-instruction metadata used in this context.
   DenseMap<const Instruction *, MDAttachmentMap> InstructionMetadata;
 
   /// Collection of per-GlobalObject metadata used in this context.
   DenseMap<const GlobalObject *, MDGlobalAttachmentMap> GlobalObjectMetadata;
 
+  /// Collection of per-GlobalObject sections used in this context.
+  DenseMap<const GlobalObject *, StringRef> GlobalObjectSections;
+
+  /// Stable collection of section strings.
+  StringSet<> SectionStrings;
+
   /// DiscriminatorTable - This table maps file:line locations to an
   /// integer representing the next DWARF path discriminator to assign to
   /// instructions in different blocks at the same location.
   DenseMap<std::pair<const char *, unsigned>, unsigned> DiscriminatorTable;
 
   int getOrAddScopeRecordIdxEntry(MDNode *N, int ExistingIdx);
   int getOrAddScopeInlinedAtIdxEntry(MDNode *Scope, MDNode *IA,int ExistingIdx);