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

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

Mostly good to me. Thanks.



================
Comment at: include/llvm/MC/MCExpr.h:56
 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) {}
----------------
Nit: Maybe move `SubclassData` to last parameter and default to `0`?



================
Comment at: include/llvm/MC/MCExpr.h:57
+  explicit MCExpr(ExprKind Kind, unsigned SubclassData, SMLoc Loc)
+    : Kind(Kind), SubclassData(SubclassData), Loc(Loc) {}
 
----------------
Call setter to validate data instead?


================
Comment at: include/llvm/MC/MCExpr.h:64
 
+  unsigned getSubclassData() const { return SubclassData; }
+
----------------
Can we add one setter `setSubclassData()` as well? We can validate the length of data there too.


================
Comment at: include/llvm/MC/MCExpr.h:139
 
-  explicit MCConstantExpr(int64_t Value)
-      : MCExpr(MCExpr::Constant, SMLoc()), Value(Value) {}
+  static const unsigned PrintInHexBit = 1 << 8;
 
----------------
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? 


================
Comment at: include/llvm/MC/MCExpr.h:143
+    assert(SizeInBytes <= 8 && "Excessive size");
+    return SizeInBytes | (PrintInHex ? PrintInHexBit : 0);
+  }
----------------
Should we avoid clobberring by `SizeInBytes & SizeInBytesBits` as well?


================
Comment at: include/llvm/MC/MCExpr.h:325
   /// Specifies how the variant kind should be printed.
-  const unsigned UseParensForSymbolVariant : 1;
+  static const unsigned UseParensForSymbolVariantBit = 1 << 16;
 
----------------
Similar to above, maybe also define `KindBits` , avoid magic number `16` here , and comments.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77939/new/

https://reviews.llvm.org/D77939