[clang][Interp] Implement function calls

[tbaeder]

Summary

Add a Call op and use the existing infrastructure for parameters and return values.

[tbaeder]

clang/lib/AST/Interp/Interp.cpp

60

This patch adds two const_casts. They aren't strictly necessary, since the InterpFrame doesn't need to modify the function. I have a local NFC patch to add const to a few places to make this work.

[shafik]

clang/test/AST/Interp/functions.cpp

38

I would expect a lot more test cases: trailing return types, default arguments, overloads etc...

[tbaeder]

clang/test/AST/Interp/functions.cpp

38

Things like trailing return types shouldn't make a difference here, provided they result in the same FunctionDecl. I've added support for CXXDefaultArgExprs.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

603

614

Should this be using CallExpr::getCallReturnType() instead? (with test cases demonstrating the difference between getType() and this call)

clang/lib/AST/Interp/Interp.cpp

60

Thanks! FWIW, because this is an entirely new interface for the compiler, I'm hoping we can force some better const correctness in this part of the code base.

409–411

Spurious whitespace change?

clang/lib/AST/Interp/InterpFrame.cpp

174

Can you add a message to this assert so folks know what broke when you got here. Also, what about functions with no arguments?

clang/lib/AST/Interp/PrimType.h

85

Spurious whitespace change?

clang/test/AST/Interp/functions.cpp

2–3

I don't think we need to limit the testing to just c++14

9–10

Why testing twice?

49

It's a bit fuzzy where to draw the line on test coverage as this example is partially about function calls and partially about reference tracking. But I think we need more complex tests than what we've got so far.

constexpr void func(int &ref) {
  ref = 12;
}
constexpr int do_it() {
  int mutate_me = 0;
  func(mutate_me);
  return mutate_me;
}
static_assert(do_it() == 12, "");

constexpr int &get_ref(int &i) {
  return i;
}
constexpr int do_it_again() {
  int i = 0;
  get_ref(i) = 12;
  return i;
}
static_assert(do_it_again() == 12, "");

(Also, while working on this interpreter, please keep in mind that C is also looking to add support for constant expressions; C2x will have constexpr object definitions and we expect to widen that functionality for C2y. But this means you should keep in mind the crazy things that C can do too, like:

int oh_god_this_hurts(struct { int a; } s) {
  return s.a;
}

which could someday get a constexpr slapped in front of it.)

Speaking of C, remember that we also have C extensions, like using VM types, that should have test coverage:

constexpr int func(int n, int array[static n], int m) {
  return array[m];
}

constexpr int do_it() {
  int array[] = { 0, 1, 2, 3, 4 };
  return func(5, array, 1);
}

static_assert(do_it() == 1, "");

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

614

Pretty sure yes, I just didn't know about it. Can the difference only be demonstrated by using references?

clang/lib/AST/Interp/Interp.cpp

409–411

Yes, but again on purpose.

clang/lib/AST/Interp/InterpFrame.cpp

174

I'll move it into stackRef(), where the problem happened and where it's clear that Args should really be set at this point.

clang/lib/AST/Interp/PrimType.h

85

It's on purpose, I was just trying to improve this part. But I can remove it.

clang/test/AST/Interp/functions.cpp

2–3

Isn't it now just testing whatever the default is when the test is run?

9–10

The first call will compile the function in ByteCodeExprGen::VisitCallExpr() and execute it, the second one checks that visiting the same call again works. I can't test that the result is actually cached and that the compilation only happens once.

49

TBH I think references can be implemented and tested in a separate, later patch. Since I haven't tested them at all until now.

[tbaeder]

I noticed two problems:

1. Calls returning void didn't work. That needed another new opcode
2. When creating a new stackframe and running Intepret()... there's nothing actually stopping the interpreter after that stack frame. This triggered the assert(VoidResult.isAbsent()) assert I added. I modified the opcode generator to return from the enclosing function if CanReturn = 1.

[aaron.ballman]

It looks like precommit CI found some relevant failures.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

614

I believe it's just references, but would have to go trace the logic through the compiler to be sure.

clang/lib/AST/Interp/PrimType.h

85

It's the only change to the file, so it should be removed (you can make an NFC commit to fix all the whitespace issues though!)

clang/test/AST/Interp/functions.cpp

2–3

It is, but that's our preference for tests when possible because we don't want to lose testing in newer language modes by pegging the test to an older language mode (note, we're currently exploring changes to this so you can specify a range of standards: https://reviews.llvm.org/D131464).

9–10

Ah, that's good to know! That might be worth leaving a commend in the file so nobody thinks it's redundant.

49

That's fine by me!

[aaron.ballman]

LGTM aside from some small nits you can fix when landing.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

607–610

626–627

clang/lib/AST/Interp/ByteCodeExprGen.h

public:
   ByteCodeExprGen(Context &Ctx, Program &P, Tys &&... Args)
       : Emitter(Ctx, P, Args...), Ctx(Ctx), P(P) {}
 
   // Expression visitors - result returned on stack.
   bool VisitCastExpr(const CastExpr *E);
   bool VisitIntegerLiteral(const IntegerLiteral *E);
   bool VisitParenExpr(const ParenExpr *E);
   bool VisitBinaryOperator(const BinaryOperator *E);
+  bool VisitCallExpr(const CallExpr *E);
   bool VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *E);
   bool VisitCXXNullPtrLiteralExpr(const CXXNullPtrLiteralExpr *E);
   bool VisitUnaryOperator(const UnaryOperator *E);
   bool VisitDeclRefExpr(const DeclRefExpr *E);
 
 protected:
   bool visitExpr(const Expr *E) override;
   bool visitDecl(const VarDecl *VD) override;

clang/lib/AST/Interp/ByteCodeExprGen.cpp

 //===--- ByteCodeExprGen.cpp - Code generator for expressions ---*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ByteCodeExprGen.h"
 #include "ByteCodeEmitter.h"
 #include "ByteCodeGenError.h"
+#include "ByteCodeStmtGen.h"
 #include "Context.h"
 #include "Function.h"
 #include "PrimType.h"
 #include "Program.h"
 #include "State.h"
 
 using namespace clang;
 using namespace clang::interp;
if (Optional<PrimType> T = classify(VD->getType())) {
       return this->emitRetValue(VD);
     }
   }
 
   return this->bail(VD);
 }
 
 template <class Emitter>
+bool ByteCodeExprGen<Emitter>::VisitCallExpr(const CallExpr *E) {
+  assert(!E->getBuiltinCallee() && "Builtin functions aren't supported yet");
+
+  const Decl *Callee = E->getCalleeDecl();
+  if (const auto *FuncDecl = dyn_cast_or_null<FunctionDecl>(Callee)) {
+    Function *Func = P.getFunction(FuncDecl);
+
+    // Templated functions might not have been compiled yet, so do it now.
+    if (!Func) {
+      if (auto R =
+              ByteCodeStmtGen<ByteCodeEmitter>(Ctx, P).compileFunc(FuncDecl)) {
+        Func = *R;
+      }
+    }
+    assert(Func);
+
+    if (Optional<PrimType> T = classify(E->getType())) {
+      // Put arguments on the stack
+      for (const auto *Arg : E->arguments()) {
+        if (!this->visit(Arg))
+          return false;
+      }
+
+      return this->emitCall(*T, Func, E);
+    } else {
+      assert(false && "Can't classify function return type");
+    }
+
+  } else {
+    assert(false && "We don't support non-FunctionDecl callees right now.");
+  }
+
+  return false;
+}
+
+template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitCXXBoolLiteralExpr(
     const CXXBoolLiteralExpr *E) {
   if (DiscardResult)
     return true;
 
   return this->emitConstBool(E->getValue(), E);
 }
 

clang/lib/AST/Interp/Context.h

 
 private:
   /// Runs a function.
   bool Run(State &Parent, Function *Func, APValue &Result);
 
   /// Checks a result from the interpreter.
   bool Check(State &Parent, llvm::Expected<bool> &&R);
 
-private:
   /// Current compilation context.
   ASTContext &Ctx;
   /// Interpreter stack, shared across invocations.
   InterpStack Stk;
   /// Constexpr program.
   std::unique_ptr<Program> P;
 };
 
 } // namespace interp
 } // namespace clang
 
 #endif

clang/lib/AST/Interp/EvalEmitter.cpp

 
 template <PrimType OpType> bool EvalEmitter::emitRet(const SourceInfo &Info) {
   if (!isActive())
     return true;
   using T = typename PrimConv<OpType>::T;
   return ReturnValue<T>(S.Stk.pop<T>(), Result);
 }
 
+template <PrimType OpType>
+bool EvalEmitter::emitCall(const Function *Func, const SourceInfo &Info) {
+
+  S.Current =
+      new InterpFrame(S, const_cast<Function *>(Func), S.Current, {}, {});
+  // Result of call will be on the stack and needs to be handled by the caller.
+  return Interpret(S, Result);
+}
+
 bool EvalEmitter::emitRetVoid(const SourceInfo &Info) { return true; }
 
 bool EvalEmitter::emitRetValue(const SourceInfo &Info) {
   // Method to recursively traverse composites.
   std::function<bool(QualType, const Pointer &, APValue &)> Composite;
   Composite = [this, &Composite](QualType Ty, const Pointer &Ptr, APValue &R) {
     if (auto *AT = Ty->getAs<AtomicType>())
       Ty = AT->getValueType();

clang/lib/AST/Interp/Function.h

 /// Contains links to the bytecode of the function, as well as metadata
 /// describing all arguments and stack-local variables.
 class Function {
 public:
   using ParamDescriptor = std::pair<PrimType, Descriptor *>;
 
   /// Returns the size of the function's local stack.
   unsigned getFrameSize() const { return FrameSize; }
-  /// Returns the size of the argument stackx
+  /// Returns the size of the argument stack.
   unsigned getArgSize() const { return ArgSize; }
 
   /// Returns a pointer to the start of the code.
   CodePtr getCodeBegin() const { return Code.data(); }
   /// Returns a pointer to the end of the code.
   CodePtr getCodeEnd() const { return Code.data() + Code.size(); }
 
   /// Returns the original FunctionDecl.

clang/lib/AST/Interp/Interp.cpp

if (InterpFrame *Caller = S.Current->Caller) {
     delete S.Current;
     S.Current = nullptr;
     if (!ReturnValue<T>(Ret, Result))
       return false;
   }
   return true;
 }
 
+template <PrimType Name, class T = typename PrimConv<Name>::T>
+static bool Call(InterpState &S, CodePtr &PC, APValue &Result,
+                 const Function *Func) {
+  S.Current =
+      new InterpFrame(S, const_cast<Function *>(Func), S.Current, PC, {});
+
+  return Interpret(S, Result);
+}
+
 static bool RetVoid(InterpState &S, CodePtr &PC, APValue &Result) {
   S.CallStackDepth--;
 
   assert(S.Current->getFrameOffset() == S.Stk.size() && "Invalid frame");
   if (!S.checkingPotentialConstantExpression())
     S.Current->popArgs();
 
   if (InterpFrame *Caller = S.Current->Caller) {

 
 bool CheckPure(InterpState &S, CodePtr OpPC, const CXXMethodDecl *MD) {
   if (!MD->isPure())
     return true;
   const SourceInfo &E = S.Current->getSource(OpPC);
   S.FFDiag(E, diag::note_constexpr_pure_virtual_call, 1) << MD;
   S.Note(MD->getLocation(), diag::note_declared_at);
   return false;
 }
+
 bool Interpret(InterpState &S, APValue &Result) {
   assert(!S.Current->isRoot());
   CodePtr PC = S.Current->getPC();
 
   // Empty program.
   if (!PC)
     return true;
 
   for (;;) {

clang/lib/AST/Interp/InterpFrame.cpp

Pointer InterpFrame::getParamPointer(unsigned Off) {
   }
 
   // Allocate memory to store the parameter and the block metadata.
   const auto &Desc = Func->getParamDescriptor(Off);
   size_t BlockSize = sizeof(Block) + Desc.second->getAllocSize();
   auto Memory = std::make_unique<char[]>(BlockSize);
   auto *B = new (Memory.get()) Block(Desc.second);
 
+  assert(Args);
   // Copy the initial value.
   TYPE_SWITCH(Desc.first, new (B->data()) T(stackRef<T>(Off)));
 
   // Record the param.
   Params.insert({Off, std::move(Memory)});
   return Pointer(B);
 }
 

clang/lib/AST/Interp/Opcodes.td

 def ArgSint16 : ArgType { let Name = "int16_t"; }
 def ArgUint16 : ArgType { let Name = "uint16_t"; }
 def ArgSint32 : ArgType { let Name = "int32_t"; }
 def ArgUint32 : ArgType { let Name = "uint32_t"; }
 def ArgSint64 : ArgType { let Name = "int64_t"; }
 def ArgUint64 : ArgType { let Name = "uint64_t"; }
 def ArgBool : ArgType { let Name = "bool"; }
 
-def ArgFunction : ArgType { let Name = "Function *"; }
+def ArgFunction : ArgType { let Name = "const Function *"; }
 def ArgRecord : ArgType { let Name = "Record *"; }
 
 def ArgSema : ArgType { let Name = "const fltSemantics *"; }
 
 def ArgExpr : ArgType { let Name = "const Expr *"; }
 def ArgFloatingLiteral : ArgType { let Name = "const FloatingLiteral *"; }
 def ArgCXXMethodDecl : ArgType { let Name = "const CXXMethodDecl *"; }
 def ArgFunctionDecl : ArgType { let Name = "const FunctionDecl *"; }

 def RetValue : Opcode {
   let CanReturn = 1;
   let ChangesPC = 1;
   let HasCustomEval = 1;
 }
 // [] -> EXIT
 def NoRet : Opcode {}
 
+
+def Call : Opcode {
+  let Args = [ArgFunction];
+  let Types = [AllTypeClass];
+  let ChangesPC = 1;
+  let CanReturn = 1;
+  let HasCustomEval = 1;
+  let HasGroup = 1;
+}
+
+
 //===----------------------------------------------------------------------===//
 // Frame management
 //===----------------------------------------------------------------------===//
 
 // [] -> []
 def Destroy : Opcode {
   let Args = [ArgUint32];
   let HasCustomEval = 1;

clang/lib/AST/Interp/PrimType.h

 
 } // namespace interp
 } // namespace clang
 
 /// Helper macro to simplify type switches.
 /// The macro implicitly exposes a type T in the scope of the inner block.
 #define TYPE_SWITCH_CASE(Name, B) \
   case Name: { using T = PrimConv<Name>::T; B; break; }
+
 #define TYPE_SWITCH(Expr, B)                                                   \
   do {                                                                         \
     switch (Expr) {                                                            \
       TYPE_SWITCH_CASE(PT_Sint8, B)                                            \
       TYPE_SWITCH_CASE(PT_Uint8, B)                                            \
       TYPE_SWITCH_CASE(PT_Sint16, B)                                           \
       TYPE_SWITCH_CASE(PT_Uint16, B)                                           \
       TYPE_SWITCH_CASE(PT_Sint32, B)                                           \

clang/test/AST/Interp/functions.cpp

+// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -std=c++14 -verify %s
+// RUN: %clang_cc1 -std=c++14 -verify=ref %s
+
+// expected-no-diagnostics
+// ref-no-diagnostics
+
+template<typename T> constexpr T identity(T t) { return t; }
+static_assert(identity(true), "");
+static_assert(identity(true), "");
+static_assert(!identity(false), "");
+
+
+constexpr int sub(int a, int b) {
+  return a - b;
+}
+static_assert(sub(5, 2) == 3, "");
+static_assert(sub(0, 5) == -5, "");
+
+constexpr int norm(int n) {
+  if (n >= 0) {
+    return identity(n);
+  }
+  return -identity(n);
+}
+static_assert(norm(5) == norm(-5), "");
+
+constexpr int square(int n) {
+  return norm(n) * norm(n);
+}
+static_assert(square(2) == 4, "");
+
+constexpr int add_second(int a, int b, bool doAdd) {
+  if (doAdd)
+    return a + b;
+  return a;
+}
+static_assert(add_second(10, 3, true) == 13, "");
+static_assert(add_second(300, -20, false) == 300, "");