[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

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

Alternatively, use static_cast<unsigned>

365

Nit: no space after (VariantKind).

Alternatively, use static_cast<VariantKind>

433

ditto

589

ditto

[nikic]

Fix coding style: Remove space after casts.

[jsji]

Mostly good to me. Thanks.

include/llvm/MC/MCExpr.h

56

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

57

Call setter to validate data instead?

64

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

139

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

Should we avoid clobberring by SizeInBytes & SizeInBytesBits as well?

325

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

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

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

[MaskRay]

LGTM.

[jsji]

include/llvm/MC/MCExpr.h

46

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

143

Yes, but how about non-assert build?

[nikic]

include/llvm/MC/MCExpr.h

46

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

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))?

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:
   ExprKind Kind;
+  /// Field reserved for use by MCExpr subclasses.
+  unsigned SubclassData : 24;
   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, unsigned SubclassData, SMLoc Loc)
+    : Kind(Kind), SubclassData(SubclassData), Loc(Loc) {}
 
   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) {}
+  static const unsigned PrintInHexBit = 1 << 8;
 
-  MCConstantExpr(int64_t Value, bool PrintInHex)
-      : MCExpr(MCExpr::Constant, SMLoc()), Value(Value),
-        PrintInHex(PrintInHex) {}
+  static unsigned encodeSubclassData(bool PrintInHex, unsigned SizeInBytes) {
+    assert(SizeInBytes <= 8 && "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, encodeSubclassData(PrintInHex, SizeInBytes),
+               SMLoc()), 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() & 0xff; }
 
-  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;
 
   /// Specifies how the variant kind should be printed.
-  const unsigned UseParensForSymbolVariant : 1;
+  static const unsigned UseParensForSymbolVariantBit = 1 << 16;
 
   // FIXME: Remove this bit.
-  const unsigned HasSubsectionsViaSymbols : 1;
+  static const unsigned HasSubsectionsViaSymbolsBit = 1 << 17;
 
-  /// 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() & 0xffff);
+  }
 
   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, Op, Loc), 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, Op, Loc), 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; }
 
   /// @}
 
   static bool classof(const MCExpr *E) {
     return E->getKind() == MCExpr::Binary;
   }
 };
 
 /// This is an extension point for target-specific MCExpr subclasses to
 /// implement.
 ///
 /// NOTE: All subclasses are required to have trivial destructors because
 /// MCExprs are bump pointer allocated and not destructed.
 class MCTargetExpr : public MCExpr {
   virtual void anchor();
 
 protected:
-  MCTargetExpr() : MCExpr(Target, SMLoc()) {}
+  MCTargetExpr() : MCExpr(Target, 0, SMLoc()) {}
   virtual ~MCTargetExpr() = default;
 
 public:
   virtual void printImpl(raw_ostream &OS, const MCAsmInfo *MAI) const = 0;
   virtual bool evaluateAsRelocatableImpl(MCValue &Res,
                                          const MCAsmLayout *Layout,
                                          const MCFixup *Fixup) const = 0;
   // allow Target Expressions to be checked for equality

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,
+             encodeSubclassData(Kind, MAI->useParensForSymbolVariant(),
+                                MAI->hasSubsectionsViaSymbols()), Loc),
       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() {}