Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Paths

[clang][Interp] Start implementing builtin functions
ClosedPublic
Actions

Authored by tbaeder on Nov 5 2022, 5:13 AM.

Reviewers

Commits

Summary

Another try at this.

This time, only implement __builtin_is_constant_evaluated().

Unfortunately I can't just use clang/test/SemaCXX/builtin-is-constant-evaluated.cpp because there are some other unsupported expressions in there.

tbaeder requested review of this revision.Nov 5 2022, 5:13 AM

tschuett added inline comments.

clang/lib/AST/Interp/InterpBuiltin.cpp
15	You are allowed to use nested namespaces.

Ping

clang/lib/AST/Interp/ByteCodeExprGen.cpp
1274	This is an unrelated change, perhaps could be in an NFC commit.
clang/lib/AST/Interp/Interp.h
1312–1313	`directly` reads oddly here? Do you mean the 'immediately' definition of this?
1313	Rather than the 'if'/'else' pair, do we just want to immediately return on the failed builtin?
1315	What is going on here? Why is this not a leak?

clang/lib/AST/Interp/ByteCodeExprGen.cpp
1274	Right, sure
clang/lib/AST/Interp/Interp.h
1312–1313	Yes, that was the purpose. I can reword it.
1313	Sure, that's possible. I guess this ties into the question of whether to use a different opcode for builtin functions; but so far I have found builtin calls to be too similar to regular ones, esp. since they don't have a special AST node.
1315	The current frame is `delete`d when successfully returning the value of the function, this happens in the `Ret` (and `RetVoid`) function in `Interp.cpp`. No opinion on whether that's the best way to do it though.

clang/lib/AST/Interp/Interp.h
1315	I really dont like doing it this way, I see we're doing it the same in 1366, but if we're expecting someone else to delete a unique-ptr, we should give them ownership of the whole unique-ptr. This mechanism is just obscuring the ownership semantics that unique_ptr is supposed to imply.

clang/lib/AST/Interp/Interp.h
1315	I blame @aaron.ballman :) I'm gonna write it on my list and create a follow-up patch.

Split CallBI out from Call, so we don't need special cases in CheckCallable etc. for builtin functions.

This revision is now accepted and ready to land.Dec 1 2022, 6:26 AM

clang/lib/AST/Interp/Interp.h
1332	We don't have to update `S.Current`?
clang/lib/AST/Interp/InterpBuiltin.cpp
21	Why not just factor out `Ret` now?

tbaeder marked 2 inline comments as done.Jan 12 2023, 3:43 AM

tbaeder added inline comments.

clang/lib/AST/Interp/Interp.h
1332	Nope, `Ret` does that.
clang/lib/AST/Interp/InterpBuiltin.cpp
21	The two implementations are slightly different now and I wanted to see if they diverge more int he future. But I've factored them together as part of https://reviews.llvm.org/D141193