[clang] Small improvments after Adding APValue to ConstantExpr

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.

Reviewers: rsmith

Reviewed By: rsmith

Subscribers: cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D63376

llvm-svn: 364011

8 files changed, 61 insertions, 35 deletions

diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 1485046684c..1d1aaf4fb11 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -2751,12 +2751,11 @@ public:
   ///
   /// \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);
@@ -2819,10 +2818,6 @@ public:
   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.
@@ -2989,7 +2984,7 @@ private:
   // 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,
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 508817f8ad4..d44a815c869 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -998,6 +998,9 @@ public:
                                    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();
   }
@@ -1009,25 +1012,20 @@ public:
     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<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 {
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 71a7d966a7e..88aa535af27 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -332,6 +332,9 @@ protected:
     /// 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;
@@ -340,6 +343,10 @@ protected:
     /// 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 {
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 7f73531a83b..d7778a71964 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -908,7 +908,7 @@ void ASTContext::setTraversalScope(const std::vector<Decl *> &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});
 }
 
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index cc92689cb92..d342bda962d 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -239,6 +239,7 @@ 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())
@@ -249,9 +250,18 @@ ConstantExpr::getStorageKind(const APValue &Value) {
   }
 }
 
+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();
 }
 
@@ -269,8 +279,6 @@ ConstantExpr *ConstantExpr::Create(const ASTContext &Context, Expr *E,
       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;
 }
 
@@ -278,7 +286,7 @@ 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;
 }
 
@@ -296,14 +304,13 @@ ConstantExpr *ConstantExpr::CreateEmpty(const ASTContext &Context,
       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;
@@ -313,12 +320,28 @@ void ConstantExpr::MoveIntoResult(APValue &Value) {
     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:
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index c0c807b11c9..3282bc0a0af 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -8963,6 +8963,8 @@ static bool tryEvaluateBuiltinObjectSize(const Expr *E, unsigned Type,
 
 bool IntExprEvaluator::VisitConstantExpr(const ConstantExpr *E) {
   llvm::SaveAndRestore<bool> InConstantContext(Info.InConstantContext, true);
+  if (E->getResultAPValueKind() != APValue::None)
+    return Success(E->getAPValueResult(), E);
   return ExprEvaluatorBaseTy::VisitConstantExpr(E);
 }
 
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 6dda693c6ad..0fda8ea515b 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -14013,8 +14013,6 @@ Decl *Sema::BuildStaticAssertDeclaration(SourceLocation StaticAssertLoc,
     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,
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 9b1e362c262..9b09059c153 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -14538,14 +14538,13 @@ Sema::VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
     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;
   }
 
@@ -14555,8 +14554,12 @@ Sema::VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
 
   // 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) &&
+      EvalResult.Val.isInt() && !EvalResult.HasSideEffects;
+
+  if (!isa<ConstantExpr>(E))
+    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