[clang] Small improvments after Adding APValue to ConstantExpr

[Tyker]

Summary

this patch has multiple small improvements related to the APValue in ConstantExpr.

changes:

• APValue in ConstantExpr are now cleaned up using ASTContext::addDestruction instead of there own system.
• ConstantExprBits Stores the ValueKind of the result beaing stored.
• VerifyIntegerConstantExpression now stores the evaluated value in ConstantExpr.
• the Constant Evaluator uses the stored value of ConstantExpr when available.

[rsmith]

Nice cleanup!

clang/include/clang/Sema/Sema.h

10298

Do you need this new flag? No callers are passing it.

[Tyker]

clang/include/clang/Sema/Sema.h

10298

the idea behind it is that not all integral constant expression benefit from storing the result. i have not analyzed which benefit from it and which don't. adding a FIXME would have probably be more appropriate.

[rsmith]

clang/include/clang/Sema/Sema.h

10298

It's useful to have a consistent AST representation, so I'd be inclined to at least always create a ConstantExpr node. Also, once we start allowing consteval evaluations that have side effects (for example, reflection-related actions that modify the AST), I think we'll need to always store the result to ensure we never evaluate a constant expression twice (and trigger its side-effects to occur twice).

[Tyker]

made the requested changes.

clang/include/clang/Sema/Sema.h

10298

I removed the StoreResult flag.
evaluating expression only once is going to be mandatory in the future but i think it is hard to reach, semantic checking often needs intermediate values. so we will need probably need to integrate semantic checking to the expression evaluator to use its intermediate value. and do so without code duplication with the non-constant context checking.

clang/include/clang/AST/ASTContext.h

public:
 
   /// Add a deallocation callback that will be invoked when the
   /// ASTContext is destroyed.
   ///
   /// \param Callback A callback function that will be invoked on destruction.
   ///
   /// \param Data Pointer data that will be provided to the callback function
   /// when it is called.
-  void AddDeallocation(void (*Callback)(void*), void *Data);
+  void AddDeallocation(void (*Callback)(void *), void *Data) const;
 
   /// If T isn't trivially destructible, calls AddDeallocation to register it
   /// for destruction.
-  template <typename T>
-  void addDestruction(T *Ptr) {
+  template <typename T> void addDestruction(T *Ptr) const {
     if (!std::is_trivially_destructible<T>::value) {
       auto DestroyPtr = [](void *V) { static_cast<T *>(V)->~T(); };
       AddDeallocation(DestroyPtr, Ptr);
     }
   }
 
   GVALinkage GetGVALinkageForFunction(const FunctionDecl *FD) const;
   GVALinkage GetGVALinkageForVariable(const VarDecl *VD);
public:
   /// index of the parameter when it exceeds the size of the normal bitfield.
   unsigned getParameterIndex(const ParmVarDecl *D) const;
 
   /// Get the storage for the constant value of a materialized temporary
   /// of static storage duration.
   APValue *getMaterializedTemporaryValue(const MaterializeTemporaryExpr *E,
                                          bool MayCreate);
 
-  /// Adds an APValue that will be destructed during the destruction of the
-  /// ASTContext.
-  void AddAPValueCleanup(APValue *Ptr) const { APValueCleanups.push_back(Ptr); }
-
   /// Return a string representing the human readable name for the specified
   /// function declaration or file name. Used by SourceLocExpr and
   /// PredefinedExpr to cache evaluated results.
   StringLiteral *getPredefinedStringLiteralFromCache(StringRef Key) const;
 
   //===--------------------------------------------------------------------===//
   //                    Statistics
   //===--------------------------------------------------------------------===//
private:
   // FIXME: We really should have a better mechanism in the ASTContext to
   // manage running destructors for types which do variable sized allocation
   // within the AST. In some places we thread the AST bump pointer allocator
   // into the datastructures which avoids this mess during deallocation but is
   // wasteful of memory, and here we require a lot of error prone book keeping
   // in order to track and run destructors while we're tearing things down.
   using DeallocationFunctionsAndArguments =
       llvm::SmallVector<std::pair<void (*)(void *), void *>, 16>;
-  DeallocationFunctionsAndArguments Deallocations;
+  mutable DeallocationFunctionsAndArguments Deallocations;
 
   // FIXME: This currently contains the set of StoredDeclMaps used
   // by DeclContext objects.  This probably should not be in ASTContext,
   // but we include it here so that ASTContext can quickly deallocate them.
   llvm::PointerIntPair<StoredDeclsMap *, 1> LastSDM;
 
   std::vector<Decl *> TraversalScope;
   class ParentMap;

clang/include/clang/AST/Expr.h

static ConstantExpr *Create(const ASTContext &Context, Expr *E,
                               const APValue &Result);
   static ConstantExpr *Create(const ASTContext &Context, Expr *E,
                               ResultStorageKind Storage = RSK_None);
   static ConstantExpr *CreateEmpty(const ASTContext &Context,
                                    ResultStorageKind StorageKind,
                                    EmptyShell Empty);
 
   static ResultStorageKind getStorageKind(const APValue &Value);
+  static ResultStorageKind getStorageKind(const Type *T,
+                                          const ASTContext &Context);
+
   SourceLocation getBeginLoc() const LLVM_READONLY {
     return SubExpr->getBeginLoc();
   }
   SourceLocation getEndLoc() const LLVM_READONLY {
     return SubExpr->getEndLoc();
   }
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == ConstantExprClass;
   }
 
-  void SetResult(APValue Value) { MoveIntoResult(Value); }
-  void MoveIntoResult(APValue &Value);
+  void SetResult(APValue Value, const ASTContext &Context) {
+    MoveIntoResult(Value, Context);
+  }
+  void MoveIntoResult(APValue &Value, const ASTContext &Context);
 
   APValue::ValueKind getResultAPValueKind() const {
-    switch (ConstantExprBits.ResultKind) {
-    case ConstantExpr::RSK_APValue:
-      return APValueResult().getKind();
-    case ConstantExpr::RSK_Int64:
-      return APValue::Int;
-    case ConstantExpr::RSK_None:
-      return APValue::None;
-    }
-    llvm_unreachable("invalid ResultKind");
+    return static_cast<APValue::ValueKind>(ConstantExprBits.APValueKind);
   }
   ResultStorageKind getResultStorageKind() const {
     return static_cast<const ResultStorageKind>(ConstantExprBits.ResultKind);
   }
   APValue getAPValueResult() const;
+  const APValue &getResultAsAPValue() const { return APValueResult(); }
+  llvm::APSInt getResultAsAPSInt() const;
 
   // Iterators
   child_range children() { return child_range(&SubExpr, &SubExpr+1); }
   const_child_range children() const {
     return const_child_range(&SubExpr, &SubExpr + 1);
   }
 };
 

clang/include/clang/AST/Stmt.h

class ConstantExprBitfields {
     friend class ASTStmtWriter;
     friend class ConstantExpr;
 
     unsigned : NumExprBits;
 
     /// The kind of result that is trail-allocated.
     unsigned ResultKind : 2;
 
+    /// Kind of Result as defined by APValue::Kind
+    unsigned APValueKind : 4;
+
     /// When ResultKind == RSK_Int64. whether the trail-allocated integer is
     /// signed.
     unsigned IsUnsigned : 1;
 
     /// When ResultKind == RSK_Int64. the BitWidth of the trail-allocated
     /// integer. 7 bits because it is the minimal number of bit to represent a
     /// value from 0 to 64 (the size of the trail-allocated number).
     unsigned BitWidth : 7;
+
+    /// When ResultKind == RSK_APValue. Wether the ASTContext will cleanup the
+    /// destructor on the trail-allocated APValue.
+    unsigned HasCleanup : 1;
   };
 
   class PredefinedExprBitfields {
     friend class ASTStmtReader;
     friend class PredefinedExpr;
 
     unsigned : NumExprBits;
 

clang/include/clang/Sema/Sema.h

public:
     virtual ~VerifyICEDiagnoser() { }
   };
 
   /// VerifyIntegerConstantExpression - Verifies that an expression is an ICE,
   /// and reports the appropriate diagnostics. Returns false on success.
   /// Can optionally return the value of the expression.
   ExprResult VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
                                              VerifyICEDiagnoser &Diagnoser,
-                                             bool AllowFold = true);
+                                             bool AllowFold = true,
+                                             bool StoreResult = true);
   ExprResult VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
                                              unsigned DiagID,
                                              bool AllowFold = true);
   ExprResult VerifyIntegerConstantExpression(Expr *E,
                                              llvm::APSInt *Result = nullptr);
 
   /// VerifyBitField - verifies that a bit field expression is an ICE and has
   /// the correct width, and that the field type is valid.

clang/lib/AST/ASTContext.cpp

public:
   }
 };
 
 void ASTContext::setTraversalScope(const std::vector<Decl *> &TopLevelDecls) {
   TraversalScope = TopLevelDecls;
   Parents.reset();
 }
 
-void ASTContext::AddDeallocation(void (*Callback)(void*), void *Data) {
+void ASTContext::AddDeallocation(void (*Callback)(void *), void *Data) const {
   Deallocations.push_back({Callback, Data});
 }
 
 void
 ASTContext::setExternalSource(IntrusiveRefCntPtr<ExternalASTSource> Source) {
   ExternalSource = std::move(Source);
 }
 

clang/lib/AST/Expr.cpp

assert((Kind == ConstantExpr::RSK_APValue ||
           Kind == ConstantExpr::RSK_Int64 || Kind == ConstantExpr::RSK_None) &&
          "Invalid StorageKind Value");
 }
 
 ConstantExpr::ResultStorageKind
 ConstantExpr::getStorageKind(const APValue &Value) {
   switch (Value.getKind()) {
   case APValue::None:
+  case APValue::Indeterminate:
     return ConstantExpr::RSK_None;
   case APValue::Int:
     if (!Value.getInt().needsCleanup())
       return ConstantExpr::RSK_Int64;
     LLVM_FALLTHROUGH;
   default:
     return ConstantExpr::RSK_APValue;
   }
 }
 
+ConstantExpr::ResultStorageKind
+ConstantExpr::getStorageKind(const Type *T, const ASTContext &Context) {
+  if (T->isIntegralOrEnumerationType() && Context.getTypeInfo(T).Width <= 64)
+    return ConstantExpr::RSK_Int64;
+  return ConstantExpr::RSK_APValue;
+}
+
 void ConstantExpr::DefaultInit(ResultStorageKind StorageKind) {
   ConstantExprBits.ResultKind = StorageKind;
-  if (StorageKind == RSK_APValue)
+  ConstantExprBits.APValueKind = APValue::None;
+  ConstantExprBits.HasCleanup = false;
+  if (StorageKind == ConstantExpr::RSK_APValue)
     ::new (getTrailingObjects<APValue>()) APValue();
 }
 
 ConstantExpr::ConstantExpr(Expr *subexpr, ResultStorageKind StorageKind)
     : FullExpr(ConstantExprClass, subexpr) {
   DefaultInit(StorageKind);
 }
 
 ConstantExpr *ConstantExpr::Create(const ASTContext &Context, Expr *E,
                                    ResultStorageKind StorageKind) {
   assert(!isa<ConstantExpr>(E));
   AssertResultStorageKind(StorageKind);
   unsigned Size = totalSizeToAlloc<APValue, uint64_t>(
       StorageKind == ConstantExpr::RSK_APValue,
       StorageKind == ConstantExpr::RSK_Int64);
   void *Mem = Context.Allocate(Size, alignof(ConstantExpr));
   ConstantExpr *Self = new (Mem) ConstantExpr(E, StorageKind);
-  if (StorageKind == ConstantExpr::RSK_APValue)
-    Context.AddAPValueCleanup(&Self->APValueResult());
   return Self;
 }
 
 ConstantExpr *ConstantExpr::Create(const ASTContext &Context, Expr *E,
                                    const APValue &Result) {
   ResultStorageKind StorageKind = getStorageKind(Result);
   ConstantExpr *Self = Create(Context, E, StorageKind);
-  Self->SetResult(Result);
+  Self->SetResult(Result, Context);
   return Self;
 }
 
 ConstantExpr::ConstantExpr(ResultStorageKind StorageKind, EmptyShell Empty)
     : FullExpr(ConstantExprClass, Empty) {
   DefaultInit(StorageKind);
 }
 
 ConstantExpr *ConstantExpr::CreateEmpty(const ASTContext &Context,
                                         ResultStorageKind StorageKind,
                                         EmptyShell Empty) {
   AssertResultStorageKind(StorageKind);
   unsigned Size = totalSizeToAlloc<APValue, uint64_t>(
       StorageKind == ConstantExpr::RSK_APValue,
       StorageKind == ConstantExpr::RSK_Int64);
   void *Mem = Context.Allocate(Size, alignof(ConstantExpr));
   ConstantExpr *Self = new (Mem) ConstantExpr(StorageKind, Empty);
-  if (StorageKind == ConstantExpr::RSK_APValue)
-    Context.AddAPValueCleanup(&Self->APValueResult());
   return Self;
 }
 
-void ConstantExpr::MoveIntoResult(APValue &Value) {
+void ConstantExpr::MoveIntoResult(APValue &Value, const ASTContext &Context) {
   assert(getStorageKind(Value) == ConstantExprBits.ResultKind &&
          "Invalid storage for this value kind");
+  ConstantExprBits.APValueKind = Value.getKind();
   switch (ConstantExprBits.ResultKind) {
   case RSK_None:
     return;
   case RSK_Int64:
     Int64Result() = *Value.getInt().getRawData();
     ConstantExprBits.BitWidth = Value.getInt().getBitWidth();
     ConstantExprBits.IsUnsigned = Value.getInt().isUnsigned();
     return;
   case RSK_APValue:
+    if (!ConstantExprBits.HasCleanup && Value.needsCleanup()) {
+      ConstantExprBits.HasCleanup = true;
+      Context.addDestruction(&APValueResult());
+    }
     APValueResult() = std::move(Value);
     return;
   }
   llvm_unreachable("Invalid ResultKind Bits");
 }
 
+llvm::APSInt ConstantExpr::getResultAsAPSInt() const {
+  switch (ConstantExprBits.ResultKind) {
+  case ConstantExpr::RSK_APValue:
+    return APValueResult().getInt();
+  case ConstantExpr::RSK_Int64:
+    return llvm::APSInt(llvm::APInt(ConstantExprBits.BitWidth, Int64Result()),
+                        ConstantExprBits.IsUnsigned);
+  default:
+    llvm_unreachable("invalid Accessor");
+  }
+}
+
 APValue ConstantExpr::getAPValueResult() const {
   switch (ConstantExprBits.ResultKind) {
   case ConstantExpr::RSK_APValue:
     return APValueResult();
   case ConstantExpr::RSK_Int64:
     return APValue(
         llvm::APSInt(llvm::APInt(ConstantExprBits.BitWidth, Int64Result()),
                      ConstantExprBits.IsUnsigned));

clang/lib/AST/ExprConstant.cpp

if (EndOffset <= LVal.getLValueOffset())
     Size = 0;
   else
     Size = (EndOffset - LVal.getLValueOffset()).getQuantity();
   return true;
 }
 
 bool IntExprEvaluator::VisitConstantExpr(const ConstantExpr *E) {
   llvm::SaveAndRestore<bool> InConstantContext(Info.InConstantContext, true);
+  if (E->getResultStorageKind() != ConstantExpr::RSK_None)
+    return Success(E->getAPValueResult(), E);
   return ExprEvaluatorBaseTy::VisitConstantExpr(E);
 }
 
 bool IntExprEvaluator::VisitCallExpr(const CallExpr *E) {
   if (unsigned BuiltinOp = E->getBuiltinCallee())
     return VisitBuiltinCallExpr(E, BuiltinOp);
 
   return ExprEvaluatorBaseTy::VisitCallExpr(E);

clang/lib/Sema/SemaDeclCXX.cpp

Decl *Sema::BuildStaticAssertDeclaration(SourceLocation StaticAssertLoc,
   assert(AssertExpr != nullptr && "Expected non-null condition");
   if (!AssertExpr->isTypeDependent() && !AssertExpr->isValueDependent() &&
       !Failed) {
     // In a static_assert-declaration, the constant-expression shall be a
     // constant expression that can be contextually converted to bool.
     ExprResult Converted = PerformContextuallyConvertToBool(AssertExpr);
     if (Converted.isInvalid())
       Failed = true;
-    else
-      Converted = ConstantExpr::Create(Context, Converted.get());
 
     llvm::APSInt Cond;
     if (!Failed && VerifyIntegerConstantExpression(Converted.get(), &Cond,
           diag::err_static_assert_expression_is_not_constant,
           /*AllowFold=*/false).isInvalid())
       Failed = true;
 
     if (!Failed && !Cond) {

clang/lib/Sema/SemaExpr.cpp

ExprResult Sema::VerifyIntegerConstantExpression(Expr *E,
   return VerifyIntegerConstantExpression(E, Result, Diagnoser, AllowFold);
 }
 
 void Sema::VerifyICEDiagnoser::diagnoseFold(Sema &S, SourceLocation Loc,
                                             SourceRange SR) {
   S.Diag(Loc, diag::ext_expr_not_ice) << SR << S.LangOpts.CPlusPlus;
 }
 
-ExprResult
-Sema::VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
+ExprResult Sema::VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
                                                  VerifyICEDiagnoser &Diagnoser,
-                                      bool AllowFold) {
+                                                 bool AllowFold,
+                                                 bool StoreResult) {
   SourceLocation DiagLoc = E->getBeginLoc();
 
   if (getLangOpts().CPlusPlus11) {
     // C++11 [expr.const]p5:
     //   If an expression of literal class type is used in a context where an
     //   integral constant expression is required, then that class type shall
     //   have a single non-explicit conversion function to an integral or
     //   unscoped enumeration type
if (!E->getType()->isIntegralOrUnscopedEnumerationType())
       return ExprError();
   } else if (!E->getType()->isIntegralOrUnscopedEnumerationType()) {
     // An ICE must be of integral or unscoped enumeration type.
     if (!Diagnoser.Suppress)
       Diagnoser.diagnoseNotICE(*this, DiagLoc, E->getSourceRange());
     return ExprError();
   }
 
-  if (!isa<ConstantExpr>(E))
-    E = ConstantExpr::Create(Context, E);
-
   // Circumvent ICE checking in C++11 to avoid evaluating the expression twice
   // in the non-ICE case.
   if (!getLangOpts().CPlusPlus11 && E->isIntegerConstantExpr(Context)) {
     if (Result)
       *Result = E->EvaluateKnownConstIntCheckOverflow(Context);
+    if (!isa<ConstantExpr>(E))
+      E = ConstantExpr::Create(Context, E);
     return E;
   }
 
   Expr::EvalResult EvalResult;
   SmallVector<PartialDiagnosticAt, 8> Notes;
   EvalResult.Diag = &Notes;
 
   // Try to evaluate the expression, and produce diagnostics explaining why it's
   // not a constant expression as a side-effect.
-  bool Folded = E->EvaluateAsRValue(EvalResult, Context) &&
-                EvalResult.Val.isInt() && !EvalResult.HasSideEffects;
+  bool Folded =
+      E->EvaluateAsRValue(EvalResult, Context, /*isConstantContext*/ true) &&
 
+  if (!isa<ConstantExpr>(E))
+    if (StoreResult)
+      E = ConstantExpr::Create(Context, E, EvalResult.Val);
+
   // In C++11, we can rely on diagnostics being produced for any expression
   // which is not a constant expression. If no diagnostics were produced, then
   // this is a constant expression.
   if (Folded && getLangOpts().CPlusPlus11 && Notes.empty()) {
     if (Result)
       *Result = EvalResult.Val.getInt();
     return E;
   }

clang/lib/Sema/SemaOverload.cpp

if (!Result.get()->EvaluateAsConstantExpr(Eval, Usage, S.Context) ||
     // The expression can't be folded, so we can't keep it at this position in
     // the AST.
     Result = ExprError();
   } else {
     Value = Eval.Val;
 
     if (Notes.empty()) {
       // It's a constant expression.
+
+      // ExplicitBool wouldn't use the value stored in ConstantExpr.
+      if (CCE != Sema::CCEK_ExplicitBool)
         return ConstantExpr::Create(S.Context, Result.get(), Value);
+      else
+        return ConstantExpr::Create(S.Context, Result.get());
     }
   }
 
   // It's not a constant expression. Produce an appropriate diagnostic.
   if (Notes.size() == 1 &&
       Notes[0].second.getDiagID() == diag::note_invalid_subexpr_in_const_expr)
     S.Diag(Notes[0].first, diag::err_expr_not_cce) << CCE;
   else {