This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Implement function calls
ClosedPublic

Authored by tbaeder on Aug 20 2022, 2:11 AM.

Details

Summary

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

Diff Detail

Event Timeline

tbaeder created this revision.Aug 20 2022, 2:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2022, 2:11 AM
tbaeder requested review of this revision.Aug 20 2022, 2:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2022, 2:11 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder added inline comments.Aug 20 2022, 2:20 AM
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 added inline comments.Aug 21 2022, 10:00 PM
clang/test/AST/Interp/functions.cpp
38

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

tbaeder updated this revision to Diff 454386.Aug 21 2022, 11:01 PM
tbaeder added inline comments.
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.

tbaeder updated this revision to Diff 454389.Aug 21 2022, 11:17 PM
aaron.ballman added inline comments.Aug 22 2022, 10:21 AM
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 marked 4 inline comments as done.Aug 22 2022, 10:27 PM
tbaeder added inline comments.
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 updated this revision to Diff 454723.Aug 23 2022, 12:52 AM
tbaeder marked 3 inline comments as done.

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.
tbaeder updated this revision to Diff 454726.Aug 23 2022, 1:01 AM

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!

tbaeder updated this revision to Diff 455791.Aug 25 2022, 9:37 PM
tbaeder marked 8 inline comments as done.
aaron.ballman accepted this revision.Aug 26 2022, 5:47 AM

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

clang/lib/AST/Interp/ByteCodeExprGen.cpp
607–610
626–627
This revision is now accepted and ready to land.Aug 26 2022, 5:47 AM
tbaeder marked 2 inline comments as done.Aug 26 2022, 5:50 AM
This revision was automatically updated to reflect the committed changes.