[MC] Use subclass data for MCExpr to reduce memory usage

[nikic]

Summary

MCExpr has a bunch of free space that is currently going to waste. Repurpose it as 24 bits of subclass data, which is enough to reduce the size of all subclasses by 8 bytes. This gives us some respectable savings for debuginfo builds. Here are the max-rss reductions for the fat LTO link step (full data):

kc.link 		238MiB 	231MiB (-2.82%)
sqlite3.link 		258MiB 	250MiB (-3.27%)
consumer-typeset.link 	152MiB 	148MiB (-2.51%)
bullet.link 		197MiB 	192MiB (-2.30%)
tramp3d-v4.link 	578MiB 	567MiB (-1.92%)
pairlocalalign.link 	92MiB 	 90MiB (-1.98%)
clamscan.link 		230MiB 	223MiB (-2.81%)
lencod.link 		242MiB 	235MiB (-2.67%)
SPASS.link 		235MiB 	230MiB (-2.23%)
7zip-benchmark.link 	450MiB 	435MiB (-3.25%)

[nikic]

I find it hard to determine who should review this, seems like all the major contributors to this code are no longer active (including the code owner of the MC layer...)

[MaskRay]

Thanks for the change! It is good. I only got some nits.

include/llvm/MC/MCExpr.h

333 ↗

(On Diff #256759)

Nit: no space after (unsigned). No outer parentheses.

Alternatively, use static_cast<unsigned>

365 ↗

(On Diff #256759)

Nit: no space after (VariantKind).

Alternatively, use static_cast<VariantKind>

433 ↗

(On Diff #256759)

ditto

589 ↗

(On Diff #256759)

ditto

[nikic]

Fix coding style: Remove space after casts.

[jsji]

Mostly good to me. Thanks.

include/llvm/MC/MCExpr.h

56 ↗

(On Diff #256794)

Nit: Maybe move SubclassData to last parameter and default to 0?

57 ↗

(On Diff #256794)

Call setter to validate data instead?

64 ↗

(On Diff #256794)

Can we add one setter setSubclassData() as well? We can validate the length of data there too.

139 ↗

(On Diff #256794)

Can we also define SizeInBytesBits ?

Can we avoid the magic number 8 here?

Also add some comments about how SizeInBytes and PrintInHexBit are encoded in SubclassData? eg: endian order?

143 ↗

(On Diff #256794)

Should we avoid clobberring by SizeInBytes & SizeInBytesBits as well?

325 ↗

(On Diff #256794)

Similar to above, maybe also define KindBits , avoid magic number 16 here , and comments.

[nikic]

• Make SubclassData last ctor parameter and optional.
• Add assertion that SubclassData is not too large.
• Replace various magic numbers with constants.

include/llvm/MC/MCExpr.h

57 ↗

(On Diff #256794)

MCExprs are immutable, so I don't think we should add a setter (and we don't have them for other fields either). However, I've added an assertion in the constructor to check that the subclass data fits.

143 ↗

(On Diff #256794)

The assertion here is intended to make sure that clobbering can't happen.

[MaskRay]

LGTM.

[jsji]

include/llvm/MC/MCExpr.h

46 ↗

(On Diff #257095)

Why we choose enum here?
Can we set it to const calculated from sizeof(unsigned int)?

143 ↗

(On Diff #256794)

Yes, but how about non-assert build?

[nikic]

include/llvm/MC/MCExpr.h

46 ↗

(On Diff #257095)

Why we choose enum here?

I'm actually not sure... I followed what I saw other similar code doing. My assumption was that some compilers do not allow using static const in bitfield widths (as they aren't "real" constants), but after some googling around I didn't find any evidence of that, so I'll switch this to use static const and go back to the enum if it breaks any bots.

Can we set it to const calculated from sizeof(unsigned int)?

Something like CHAR_BIT * (sizeof(unsigned int) - sizeof(uint8_t))? I'm not sure this is better, because it makes it non-obvious how much space is actually free to use.

[jsji]

include/llvm/MC/MCExpr.h

46 ↗

(On Diff #257095)

Something like CHAR_BIT * (sizeof(unsigned int) - sizeof(uint8_t))? I'm not sure this is better, because it makes it non-obvious how much space is actually free to use.

OK, make sense too. Maybe add a comments to 24 == CHAR_BIT * (sizeof(unsigned int) - sizeof(ExprKind))?

llvm/include/llvm/MC/MCExpr.h

 class StringRef;
 
 using SectionAddrMap = DenseMap<const MCSection *, uint64_t>;
 
 /// Base class for the full range of assembler expressions which are
 /// needed for parsing.
 class MCExpr {
 public:
-  enum ExprKind {
+  enum ExprKind : uint8_t {
     Binary,    ///< Binary expressions.
     Constant,  ///< Constant expressions.
     SymbolRef, ///< References to labels and assigned expressions.
     Unary,     ///< Unary expressions.
     Target     ///< Target specific expression.
   };
 
 private:
+  static const unsigned NumSubclassDataBits = 24;
+  static_assert(
+      NumSubclassDataBits == CHAR_BIT * (sizeof(unsigned) - sizeof(ExprKind)),
+      "ExprKind and SubclassData together should take up one word");
+
   ExprKind Kind;
+  /// Field reserved for use by MCExpr subclasses.
+  unsigned SubclassData : NumSubclassDataBits;
   SMLoc Loc;
 
   bool evaluateAsAbsolute(int64_t &Res, const MCAssembler *Asm,
                           const MCAsmLayout *Layout,
                           const SectionAddrMap *Addrs, bool InSet) const;
 
 protected:
-  explicit MCExpr(ExprKind Kind, SMLoc Loc) : Kind(Kind), Loc(Loc) {}
+  explicit MCExpr(ExprKind Kind, SMLoc Loc, unsigned SubclassData = 0)
+      : Kind(Kind), SubclassData(SubclassData), Loc(Loc) {
+    assert(SubclassData < (1 << NumSubclassDataBits) &&
+           "Subclass data too large");
+  }
 
   bool evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
                                  const MCAsmLayout *Layout,
                                  const MCFixup *Fixup,
                                  const SectionAddrMap *Addrs, bool InSet) const;
 
+  unsigned getSubclassData() const { return SubclassData; }
+
 public:
   MCExpr(const MCExpr &) = delete;
   MCExpr &operator=(const MCExpr &) = delete;
 
   /// \name Accessors
   /// @{
 
   ExprKind getKind() const { return Kind; }

 inline raw_ostream &operator<<(raw_ostream &OS, const MCExpr &E) {
   E.print(OS, nullptr);
   return OS;
 }
 
 ////  Represent a constant integer expression.
 class MCConstantExpr : public MCExpr {
   int64_t Value;
-  bool PrintInHex = false;
-  unsigned SizeInBytes = 0;
-
-  explicit MCConstantExpr(int64_t Value)
-      : MCExpr(MCExpr::Constant, SMLoc()), Value(Value) {}
 
-  MCConstantExpr(int64_t Value, bool PrintInHex)
-      : MCExpr(MCExpr::Constant, SMLoc()), Value(Value),
-        PrintInHex(PrintInHex) {}
+  // Subclass data stores SizeInBytes in bits 0..7 and PrintInHex in bit 8.
+  static const unsigned SizeInBytesBits = 8;
+  static const unsigned SizeInBytesMask = (1 << SizeInBytesBits) - 1;
+  static const unsigned PrintInHexBit = 1 << SizeInBytesBits;
+
+  static unsigned encodeSubclassData(bool PrintInHex, unsigned SizeInBytes) {
+    assert(SizeInBytes <= sizeof(int64_t) && "Excessive size");
+    return SizeInBytes | (PrintInHex ? PrintInHexBit : 0);
+  }
 
   MCConstantExpr(int64_t Value, bool PrintInHex, unsigned SizeInBytes)
-      : MCExpr(MCExpr::Constant, SMLoc()), Value(Value), PrintInHex(PrintInHex),
-        SizeInBytes(SizeInBytes) {}
+      : MCExpr(MCExpr::Constant, SMLoc(),
+               encodeSubclassData(PrintInHex, SizeInBytes)), Value(Value) {}
 
 public:
   /// \name Construction
   /// @{
 
   static const MCConstantExpr *create(int64_t Value, MCContext &Ctx,
                                       bool PrintInHex = false,
                                       unsigned SizeInBytes = 0);
 
   /// @}
   /// \name Accessors
   /// @{
 
   int64_t getValue() const { return Value; }
-  unsigned getSizeInBytes() const { return SizeInBytes; }
+  unsigned getSizeInBytes() const {
+    return getSubclassData() & SizeInBytesMask;
+  }
 
-  bool useHexFormat() const { return PrintInHex; }
+  bool useHexFormat() const { return (getSubclassData() & PrintInHexBit) != 0; }
 
   /// @}
 
   static bool classof(const MCExpr *E) {
     return E->getKind() == MCExpr::Constant;
   }
 };
 
enum VariantKind : uint16_t {
     VK_AMDGPU_ABS32_LO,      // symbol@abs32@lo
     VK_AMDGPU_ABS32_HI,      // symbol@abs32@hi
 
     VK_TPREL,
     VK_DTPREL
   };
 
 private:
-  /// The symbol reference modifier.
-  const VariantKind Kind;
+  /// The symbol being referenced.
+  const MCSymbol *Symbol;
+
+  // Subclass data stores VariantKind in bits 0..15, UseParensForSymbolVariant
+  // in bit 16 and HasSubsectionsViaSymbols in bit 17.
+  static const unsigned VariantKindBits = 16;
+  static const unsigned VariantKindMask = (1 << VariantKindBits) - 1;
 
   /// Specifies how the variant kind should be printed.
-  const unsigned UseParensForSymbolVariant : 1;
+  static const unsigned UseParensForSymbolVariantBit = 1 << VariantKindBits;
 
   // FIXME: Remove this bit.
-  const unsigned HasSubsectionsViaSymbols : 1;
+  static const unsigned HasSubsectionsViaSymbolsBit =
+      1 << (VariantKindBits + 1);
 
-  /// The symbol being referenced.
-  const MCSymbol *Symbol;
+  static unsigned encodeSubclassData(VariantKind Kind,
+                              bool UseParensForSymbolVariant,
+                              bool HasSubsectionsViaSymbols) {
+    return (unsigned)Kind |
+           (UseParensForSymbolVariant ? UseParensForSymbolVariantBit : 0) |
+           (HasSubsectionsViaSymbols ? HasSubsectionsViaSymbolsBit : 0);
+  }
+
+  bool useParensForSymbolVariant() const {
+    return (getSubclassData() & UseParensForSymbolVariantBit) != 0;
+  }
 
   explicit MCSymbolRefExpr(const MCSymbol *Symbol, VariantKind Kind,
                            const MCAsmInfo *MAI, SMLoc Loc = SMLoc());
 
 public:
   /// \name Construction
   /// @{
 
   static const MCSymbolRefExpr *create(const MCSymbol *Symbol, MCContext &Ctx) {
     return MCSymbolRefExpr::create(Symbol, VK_None, Ctx);
   }
 
   static const MCSymbolRefExpr *create(const MCSymbol *Symbol, VariantKind Kind,
                                        MCContext &Ctx, SMLoc Loc = SMLoc());
   static const MCSymbolRefExpr *create(StringRef Name, VariantKind Kind,
                                        MCContext &Ctx);
 
   /// @}
   /// \name Accessors
   /// @{
 
   const MCSymbol &getSymbol() const { return *Symbol; }
 
-  VariantKind getKind() const { return Kind; }
+  VariantKind getKind() const {
+    return (VariantKind)(getSubclassData() & VariantKindMask);
+  }
 
   void printVariantKind(raw_ostream &OS) const;
 
-  bool hasSubsectionsViaSymbols() const { return HasSubsectionsViaSymbols; }
+  bool hasSubsectionsViaSymbols() const {
+    return (getSubclassData() & HasSubsectionsViaSymbolsBit) != 0;
+  }
 
   /// @}
   /// \name Static Utility Functions
   /// @{
 
   static StringRef getVariantKindName(VariantKind Kind);
 
   static VariantKind getVariantKindForName(StringRef Name);
public:
   enum Opcode {
     LNot,  ///< Logical negation.
     Minus, ///< Unary minus.
     Not,   ///< Bitwise negation.
     Plus   ///< Unary plus.
   };
 
 private:
-  Opcode Op;
   const MCExpr *Expr;
 
   MCUnaryExpr(Opcode Op, const MCExpr *Expr, SMLoc Loc)
-      : MCExpr(MCExpr::Unary, Loc), Op(Op), Expr(Expr) {}
+      : MCExpr(MCExpr::Unary, Loc, Op), Expr(Expr) {}
 
 public:
   /// \name Construction
   /// @{
 
   static const MCUnaryExpr *create(Opcode Op, const MCExpr *Expr,
                                    MCContext &Ctx, SMLoc Loc = SMLoc());
 
static const MCUnaryExpr *createPlus(const MCExpr *Expr, MCContext &Ctx, SMLoc Loc = SMLoc()) {
     return create(Plus, Expr, Ctx, Loc);
   }
 
   /// @}
   /// \name Accessors
   /// @{
 
   /// Get the kind of this unary expression.
-  Opcode getOpcode() const { return Op; }
+  Opcode getOpcode() const { return (Opcode)getSubclassData(); }
 
   /// Get the child of this unary expression.
   const MCExpr *getSubExpr() const { return Expr; }
 
   /// @}
 
   static bool classof(const MCExpr *E) {
     return E->getKind() == MCExpr::Unary;
enum Opcode {
     Shl,  ///< Shift left.
     AShr, ///< Arithmetic shift right.
     LShr, ///< Logical shift right.
     Sub,  ///< Subtraction.
     Xor   ///< Bitwise exclusive or.
   };
 
 private:
-  Opcode Op;
   const MCExpr *LHS, *RHS;
 
   MCBinaryExpr(Opcode Op, const MCExpr *LHS, const MCExpr *RHS,
                SMLoc Loc = SMLoc())
-      : MCExpr(MCExpr::Binary, Loc), Op(Op), LHS(LHS), RHS(RHS) {}
+      : MCExpr(MCExpr::Binary, Loc, Op), LHS(LHS), RHS(RHS) {}
 
 public:
   /// \name Construction
   /// @{
 
   static const MCBinaryExpr *create(Opcode Op, const MCExpr *LHS,
                                     const MCExpr *RHS, MCContext &Ctx,
                                     SMLoc Loc = SMLoc());
static const MCBinaryExpr *createXor(const MCExpr *LHS, const MCExpr *RHS,
     return create(Xor, LHS, RHS, Ctx);
   }
 
   /// @}
   /// \name Accessors
   /// @{
 
   /// Get the kind of this binary expression.
-  Opcode getOpcode() const { return Op; }
+  Opcode getOpcode() const { return (Opcode)getSubclassData(); }
 
   /// Get the left-hand side expression of the binary operator.
   const MCExpr *getLHS() const { return LHS; }
 
   /// Get the right-hand side expression of the binary operator.
   const MCExpr *getRHS() const { return RHS; }
 
   /// @}

llvm/lib/MC/MCExpr.cpp

const MCConstantExpr *MCConstantExpr::create(int64_t Value, MCContext &Ctx,
                                              unsigned SizeInBytes) {
   return new (Ctx) MCConstantExpr(Value, PrintInHex, SizeInBytes);
 }
 
 /* *** */
 
 MCSymbolRefExpr::MCSymbolRefExpr(const MCSymbol *Symbol, VariantKind Kind,
                                  const MCAsmInfo *MAI, SMLoc Loc)
-    : MCExpr(MCExpr::SymbolRef, Loc), Kind(Kind),
-      UseParensForSymbolVariant(MAI->useParensForSymbolVariant()),
-      HasSubsectionsViaSymbols(MAI->hasSubsectionsViaSymbols()),
+    : MCExpr(MCExpr::SymbolRef, Loc,
+             encodeSubclassData(Kind, MAI->useParensForSymbolVariant(),
+                                MAI->hasSubsectionsViaSymbols())),
       Symbol(Symbol) {
   assert(Symbol);
 }
 
 const MCSymbolRefExpr *MCSymbolRefExpr::create(const MCSymbol *Sym,
                                                VariantKind Kind,
                                                MCContext &Ctx, SMLoc Loc) {
   return new (Ctx) MCSymbolRefExpr(Sym, Kind, Ctx.getAsmInfo(), Loc);
return StringSwitch<VariantKind>(Name.lower())
     .Case("rel32@hi", VK_AMDGPU_REL32_HI)
     .Case("rel64", VK_AMDGPU_REL64)
     .Case("abs32@lo", VK_AMDGPU_ABS32_LO)
     .Case("abs32@hi", VK_AMDGPU_ABS32_HI)
     .Default(VK_Invalid);
 }
 
 void MCSymbolRefExpr::printVariantKind(raw_ostream &OS) const {
-  if (UseParensForSymbolVariant)
+  if (useParensForSymbolVariant())
     OS << '(' << MCSymbolRefExpr::getVariantKindName(getKind()) << ')';
   else
     OS << '@' << MCSymbolRefExpr::getVariantKindName(getKind());
 }
 
 /* *** */
 
 void MCTargetExpr::anchor() {}