This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add storage for APValue in ConstantExpr
ClosedPublic

Authored by Tyker on May 24 2019, 8:55 AM.

Details

Summary

When using ConstantExpr we often need the result of the expression to be kept in the AST. Currently this is done on a by the node that needs the result and has been done multiple times for enumerator, for constexpr variables... . This patch adds to ConstantExpr the ability to store the result of evaluating the expression. no functional changes expected.

Changes:

  • Add trailling object to ConstantExpr that can hold an APValue or an uint64_t. the uint64_t is here because most ConstantExpr yield integral values so there is an optimized layout for integral values.
  • Add basic* serialization support for the trailing result.
  • Move conversion functions from an enum to a fltSemantics from clang::FloatingLiteral to llvm::APFloatBase. this change is to make it usable for serializing APValues.
  • Add basic* Import support for the trailing result.
  • ConstantExpr created in CheckConvertedConstantExpression now stores the result in the ConstantExpr Node.
  • Adapt AST dump to print the result when present.

basic* : None, Indeterminate, Int, Float, FixedPoint, ComplexInt, ComplexFloat,
the result is not yet used anywhere but for -ast-dump.

Diff Detail

Event Timeline

Tyker created this revision.May 24 2019, 8:55 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
rsmith accepted this revision.Jun 7 2019, 11:11 AM

Looks like a good first step, thanks!

clang/include/clang/AST/ASTContext.h
2821–2823

We'll need cleanups for APSInts 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 APSInts 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_ts 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.

clang/include/clang/Serialization/ASTWriter.h
866

Comment needs to be updated.

clang/lib/AST/Expr.cpp
241

After recent changes, this has been split into APValue::None and APValue::Indeterminate.

clang/lib/Serialization/ASTReader.cpp
9108–9110

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

This revision is now accepted and ready to land.Jun 7 2019, 11:11 AM
Tyker updated this revision to Diff 203723.Jun 9 2019, 12:07 AM
Tyker marked 4 inline comments as done.
Tyker edited the summary of this revision. (Show Details)

fixed requested changes.
i will commit it when i have access.

This revision was automatically updated to reflect the committed changes.