[ConstExprPreter] Removed the flag forcing the use of the interpreter

Summary:
Removed the ```-fforce-experimental-new-constant-interpreter flag```, leaving
only the ```-fexperimental-new-constant-interpreter``` one. The interpreter
now always emits an error on an unsupported feature.

Allowing the interpreter to bail out would require a mapping from APValue to
interpreter memory, which will not be necessary in the final version. It is
more sensible to always emit an error if the interpreter fails.

Reviewers: jfb, Bigcheese, rsmith, dexonsmith

Subscribers: cfe-commits

Tags: #clang

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

5 files changed, 72 insertions, 139 deletions

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index eec9bbdaef8..df80cb4f944 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -763,11 +763,8 @@ namespace {
     /// we will evaluate.
     unsigned StepsLeft;
 
-    /// Force the use of the experimental new constant interpreter, bailing out
-    /// with an error if a feature is not supported.
-    bool ForceNewConstInterp;
-
-    /// Enable the experimental new constant interpreter.
+    /// Enable the experimental new constant interpreter. If an expression is
+    /// not supported by the interpreter, an error is triggered.
     bool EnableNewConstInterp;
 
     /// BottomFrame - The frame in which evaluation started. This must be
@@ -922,9 +919,7 @@ namespace {
         : Ctx(const_cast<ASTContext &>(C)), EvalStatus(S), CurrentCall(nullptr),
           CallStackDepth(0), NextCallIndex(1),
           StepsLeft(C.getLangOpts().ConstexprStepLimit),
-          ForceNewConstInterp(C.getLangOpts().ForceNewConstInterp),
-          EnableNewConstInterp(ForceNewConstInterp ||
-                               C.getLangOpts().EnableNewConstInterp),
+          EnableNewConstInterp(C.getLangOpts().EnableNewConstInterp),
           BottomFrame(*this, SourceLocation(), nullptr, nullptr, nullptr),
           EvaluatingDecl((const ValueDecl *)nullptr),
           EvaluatingDeclValue(nullptr), HasActiveDiagnostic(false),
@@ -13400,32 +13395,25 @@ static bool EvaluateInPlace(APValue &Result, EvalInfo &Info, const LValue &This,
 /// EvaluateAsRValue - Try to evaluate this expression, performing an implicit
 /// lvalue-to-rvalue cast if it is an lvalue.
 static bool EvaluateAsRValue(EvalInfo &Info, const Expr *E, APValue &Result) {
-   if (Info.EnableNewConstInterp) {
-    auto &InterpCtx = Info.Ctx.getInterpContext();
-    switch (InterpCtx.evaluateAsRValue(Info, E, Result)) {
-    case interp::InterpResult::Success:
-      return true;
-    case interp::InterpResult::Fail:
+  if (Info.EnableNewConstInterp) {
+    if (!Info.Ctx.getInterpContext().evaluateAsRValue(Info, E, Result))
+      return false;
+  } else {
+    if (E->getType().isNull())
       return false;
-    case interp::InterpResult::Bail:
-      break;
-    }
-  }
-
-  if (E->getType().isNull())
-    return false;
-
-  if (!CheckLiteralType(Info, E))
-    return false;
 
-  if (!::Evaluate(Result, Info, E))
-    return false;
+    if (!CheckLiteralType(Info, E))
+      return false;
 
-  if (E->isGLValue()) {
-    LValue LV;
-    LV.setFrom(Info.Ctx, Result);
-    if (!handleLValueToRValueConversion(Info, E, E->getType(), LV, Result))
+    if (!::Evaluate(Result, Info, E))
       return false;
+
+    if (E->isGLValue()) {
+      LValue LV;
+      LV.setFrom(Info.Ctx, Result);
+      if (!handleLValueToRValueConversion(Info, E, E->getType(), LV, Result))
+        return false;
+    }
   }
 
   // Check this core constant expression is a constant expression.
@@ -13637,46 +13625,36 @@ bool Expr::EvaluateAsInitializer(APValue &Value, const ASTContext &Ctx,
 
   if (Info.EnableNewConstInterp) {
     auto &InterpCtx = const_cast<ASTContext &>(Ctx).getInterpContext();
-    switch (InterpCtx.evaluateAsInitializer(Info, VD, Value)) {
-    case interp::InterpResult::Fail:
-      // Bail out if an error was encountered.
-      return false;
-    case interp::InterpResult::Success:
-      // Evaluation succeeded and value was set.
-      return CheckConstantExpression(Info, DeclLoc, DeclTy, Value);
-    case interp::InterpResult::Bail:
-      // Evaluate the value again for the tree evaluator to use.
-      break;
+    if (!InterpCtx.evaluateAsInitializer(Info, VD, Value))
+      return false;
+  } else {
+    LValue LVal;
+    LVal.set(VD);
+
+    // C++11 [basic.start.init]p2:
+    //  Variables with static storage duration or thread storage duration shall
+    //  be zero-initialized before any other initialization takes place.
+    // This behavior is not present in C.
+    if (Ctx.getLangOpts().CPlusPlus && !VD->hasLocalStorage() &&
+        !DeclTy->isReferenceType()) {
+      ImplicitValueInitExpr VIE(DeclTy);
+      if (!EvaluateInPlace(Value, Info, LVal, &VIE,
+                           /*AllowNonLiteralTypes=*/true))
+        return false;
     }
-  }
-
-  LValue LVal;
-  LVal.set(VD);
 
-  // C++11 [basic.start.init]p2:
-  //  Variables with static storage duration or thread storage duration shall be
-  //  zero-initialized before any other initialization takes place.
-  // This behavior is not present in C.
-  if (Ctx.getLangOpts().CPlusPlus && !VD->hasLocalStorage() &&
-      !DeclTy->isReferenceType()) {
-    ImplicitValueInitExpr VIE(DeclTy);
-    if (!EvaluateInPlace(Value, Info, LVal, &VIE,
-                         /*AllowNonLiteralTypes=*/true))
+    if (!EvaluateInPlace(Value, Info, LVal, this,
+                         /*AllowNonLiteralTypes=*/true) ||
+        EStatus.HasSideEffects)
       return false;
-  }
-
-  if (!EvaluateInPlace(Value, Info, LVal, this,
-                       /*AllowNonLiteralTypes=*/true) ||
-      EStatus.HasSideEffects)
-    return false;
-
-  // At this point, any lifetime-extended temporaries are completely
-  // initialized.
-  Info.performLifetimeExtension();
 
-  if (!Info.discardCleanups())
-    llvm_unreachable("Unhandled cleanup; missing full expression marker?");
+    // At this point, any lifetime-extended temporaries are completely
+    // initialized.
+    Info.performLifetimeExtension();
 
+    if (!Info.discardCleanups())
+      llvm_unreachable("Unhandled cleanup; missing full expression marker?");
+  }
   return CheckConstantExpression(Info, DeclLoc, DeclTy, Value) &&
          CheckMemoryLeaks(Info);
 }
@@ -14415,14 +14393,8 @@ bool Expr::isPotentialConstantExpr(const FunctionDecl *FD,
 
   // The constexpr VM attempts to compile all methods to bytecode here.
   if (Info.EnableNewConstInterp) {
-    auto &InterpCtx = Info.Ctx.getInterpContext();
-    switch (InterpCtx.isPotentialConstantExpr(Info, FD)) {
-    case interp::InterpResult::Success:
-    case interp::InterpResult::Fail:
-      return Diags.empty();
-    case interp::InterpResult::Bail:
-      break;
-    }
+    Info.Ctx.getInterpContext().isPotentialConstantExpr(Info, FD);
+    return Diags.empty();
   }
 
   const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD);
diff --git a/clang/lib/AST/Interp/Context.cpp b/clang/lib/AST/Interp/Context.cpp
index 4f8f7b96e7c..e7f9ba0f010 100644
--- a/clang/lib/AST/Interp/Context.cpp
+++ b/clang/lib/AST/Interp/Context.cpp
@@ -21,44 +21,37 @@
 using namespace clang;
 using namespace clang::interp;
 
-Context::Context(ASTContext &Ctx)
-    : Ctx(Ctx), ForceInterp(getLangOpts().ForceNewConstInterp),
-      P(new Program(*this)) {}
+Context::Context(ASTContext &Ctx) : Ctx(Ctx), P(new Program(*this)) {}
 
 Context::~Context() {}
 
-InterpResult Context::isPotentialConstantExpr(State &Parent,
-                                              const FunctionDecl *FD) {
+bool Context::isPotentialConstantExpr(State &Parent, const FunctionDecl *FD) {
   Function *Func = P->getFunction(FD);
   if (!Func) {
     if (auto R = ByteCodeStmtGen<ByteCodeEmitter>(*this, *P).compileFunc(FD)) {
       Func = *R;
-    } else if (ForceInterp) {
+    } else {
       handleAllErrors(R.takeError(), [&Parent](ByteCodeGenError &Err) {
         Parent.FFDiag(Err.getLoc(), diag::err_experimental_clang_interp_failed);
       });
-      return InterpResult::Fail;
-    } else {
-      consumeError(R.takeError());
-      return InterpResult::Bail;
+      return false;
     }
   }
 
   if (!Func->isConstexpr())
-    return InterpResult::Fail;
+    return false;
 
   APValue Dummy;
   return Run(Parent, Func, Dummy);
 }
 
-InterpResult Context::evaluateAsRValue(State &Parent, const Expr *E,
-                                       APValue &Result) {
+bool Context::evaluateAsRValue(State &Parent, const Expr *E, APValue &Result) {
   ByteCodeExprGen<EvalEmitter> C(*this, *P, Parent, Stk, Result);
   return Check(Parent, C.interpretExpr(E));
 }
 
-InterpResult Context::evaluateAsInitializer(State &Parent, const VarDecl *VD,
-                                            APValue &Result) {
+bool Context::evaluateAsInitializer(State &Parent, const VarDecl *VD,
+                                    APValue &Result) {
   ByteCodeExprGen<EvalEmitter> C(*this, *P, Parent, Stk, Result);
   return Check(Parent, C.interpretDecl(VD));
 }
@@ -116,33 +109,20 @@ unsigned Context::getCharBit() const {
   return Ctx.getTargetInfo().getCharWidth();
 }
 
-InterpResult Context::Run(State &Parent, Function *Func, APValue &Result) {
-  InterpResult Flag;
-  {
-    InterpState State(Parent, *P, Stk, *this);
-    State.Current = new InterpFrame(State, Func, nullptr, {}, {});
-    if (Interpret(State, Result)) {
-      Flag = InterpResult::Success;
-    } else {
-      Flag = InterpResult::Fail;
-    }
-  }
-
-  if (Flag != InterpResult::Success)
-    Stk.clear();
-  return Flag;
+bool Context::Run(State &Parent, Function *Func, APValue &Result) {
+  InterpState State(Parent, *P, Stk, *this);
+  State.Current = new InterpFrame(State, Func, nullptr, {}, {});
+  if (Interpret(State, Result))
+    return true;
+  Stk.clear();
+  return false;
 }
 
-InterpResult Context::Check(State &Parent, llvm::Expected<bool> &&R) {
-  if (R) {
-    return *R ? InterpResult::Success : InterpResult::Fail;
-  } else if (ForceInterp) {
-    handleAllErrors(R.takeError(), [&Parent](ByteCodeGenError &Err) {
-      Parent.FFDiag(Err.getLoc(), diag::err_experimental_clang_interp_failed);
-    });
-    return InterpResult::Fail;
-  } else {
-    consumeError(R.takeError());
-    return InterpResult::Bail;
-  }
+bool Context::Check(State &Parent, llvm::Expected<bool> &&Flag) {
+  if (Flag)
+    return *Flag;
+  handleAllErrors(Flag.takeError(), [&Parent](ByteCodeGenError &Err) {
+    Parent.FFDiag(Err.getLoc(), diag::err_experimental_clang_interp_failed);
+  });
+  return false;
 }
diff --git a/clang/lib/AST/Interp/Context.h b/clang/lib/AST/Interp/Context.h
index 96368b6e5f0..e4d831cbb99 100644
--- a/clang/lib/AST/Interp/Context.h
+++ b/clang/lib/AST/Interp/Context.h
@@ -34,16 +34,6 @@ class Program;
 class State;
 enum PrimType : unsigned;
 
-/// Wrapper around interpreter termination results.
-enum class InterpResult {
-  /// Interpreter successfully computed a value.
-  Success,
-  /// Interpreter encountered an error and quit.
-  Fail,
-  /// Interpreter encountered an unimplemented feature, AST fallback.
-  Bail,
-};
-
 /// Holds all information required to evaluate constexpr code in a module.
 class Context {
 public:
@@ -54,15 +44,13 @@ public:
   ~Context();
 
   /// Checks if a function is a potential constant expression.
-  InterpResult isPotentialConstantExpr(State &Parent,
-                                       const FunctionDecl *FnDecl);
+  bool isPotentialConstantExpr(State &Parent, const FunctionDecl *FnDecl);
 
   /// Evaluates a toplevel expression as an rvalue.
-  InterpResult evaluateAsRValue(State &Parent, const Expr *E, APValue &Result);
+  bool evaluateAsRValue(State &Parent, const Expr *E, APValue &Result);
 
   /// Evaluates a toplevel initializer.
-  InterpResult evaluateAsInitializer(State &Parent, const VarDecl *VD,
-                                     APValue &Result);
+  bool evaluateAsInitializer(State &Parent, const VarDecl *VD, APValue &Result);
 
   /// Returns the AST context.
   ASTContext &getASTContext() const { return Ctx; }
@@ -78,16 +66,14 @@ public:
 
 private:
   /// Runs a function.
-  InterpResult Run(State &Parent, Function *Func, APValue &Result);
+  bool Run(State &Parent, Function *Func, APValue &Result);
 
   /// Checks a result fromt the interpreter.
-  InterpResult Check(State &Parent, llvm::Expected<bool> &&R);
+  bool Check(State &Parent, llvm::Expected<bool> &&R);
 
 private:
   /// Current compilation context.
   ASTContext &Ctx;
-  /// Flag to indicate if the use of the interpreter is mandatory.
-  bool ForceInterp;
   /// Interpreter stack, shared across invocations.
   InterpStack Stk;
   /// Constexpr program.
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 03ebef550cd..26d13c71467 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -4503,9 +4503,6 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
   if (Args.hasArg(options::OPT_fexperimental_new_constant_interpreter))
     CmdArgs.push_back("-fexperimental-new-constant-interpreter");
 
-  if (Args.hasArg(options::OPT_fforce_experimental_new_constant_interpreter))
-    CmdArgs.push_back("-fforce-experimental-new-constant-interpreter");
-
   if (Arg *A = Args.getLastArg(options::OPT_fbracket_depth_EQ)) {
     CmdArgs.push_back("-fbracket-depth");
     CmdArgs.push_back(A->getValue());
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 56f66537557..74831e78d8c 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -2854,8 +2854,6 @@ static void ParseLangArgs(LangOptions &Opts, ArgList &Args, InputKind IK,
       getLastArgIntValue(Args, OPT_fconstexpr_steps, 1048576, Diags);
   Opts.EnableNewConstInterp =
       Args.hasArg(OPT_fexperimental_new_constant_interpreter);
-  Opts.ForceNewConstInterp =
-      Args.hasArg(OPT_fforce_experimental_new_constant_interpreter);
   Opts.BracketDepth = getLastArgIntValue(Args, OPT_fbracket_depth, 256, Diags);
   Opts.DelayedTemplateParsing = Args.hasArg(OPT_fdelayed_template_parsing);
   Opts.NumLargeByValueCopy =