[PATCH] D62399: [clang] Add storage for APValue in ConstantExpr

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Looks like a good first step, thanks!



================
Comment at: clang/include/clang/AST/ASTContext.h:2821-2823
+  /// Adds an APValue that will be destructed during the destruction of the
+  /// ASTContext.
+  void AddAPValueCleanup(APValue *Ptr) const { APValueCleanups.push_back(Ptr); }
----------------
We'll need cleanups for `APSInt`s too, in the case where they don't fit into their inline storage. Maybe instead of storing a trailing `APSInt` we could store a trailing `uint64_t` (and put the bit-width and `Signed` flag into the `ConstantExprBits`), and use the `APValue` representation for `APSInt`s that don't fit in 64 bits? (That'd avoid needing a separate list of cleanups and would also make `ConstantExpr` 16 bytes smaller in the common case of storing an int that fits in 64 bits.)

As an alternative, we could tail-allocate a sequence of `uint64_t`s for an `APSInt` that's larger than 64 bits, but that would force a memory allocation and a copy on every access to the value, so I think it's not worthwhile.


================
Comment at: clang/include/clang/Serialization/ASTWriter.h:866
 
+  /// Emit an integral value.
+  void AddAPValue(const APValue &Value);
----------------
Comment needs to be updated.


================
Comment at: clang/lib/AST/Expr.cpp:241
+  switch (Kind) {
+  case APValue::Uninitialized:
+    return ConstantExpr::RSK_None;
----------------
After recent changes, this has been split into `APValue::None` and `APValue::Indeterminate`.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9108-9110
+  uint64_t Tmp = Record[Idx++];
+  unsigned Width = Tmp & 0xffffffff;
+  unsigned Scale = Tmp >> static_cast<uint64_t>(32);
----------------
Record values in AST files are typically stored with VBR6 encoding, as described here: https://llvm.org/docs/BitCodeFormat.html#variable-width-value

In particular, that encoding means it's usually more space-efficient to store a pair of two 32-bit integers as two separate fields rather than packing the bits of the integers into one field like this. (If you bit-pack like this, then you always use at least 7 VBR6 fields = 42 bits to store these two values, whereas if you treat them as separate fields and both are small, they can fit into 2 VBR6 fields = 12 bits.)

So please store `Width` and `Scale` as two separate vector entries :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D62399