-
Notifications
You must be signed in to change notification settings - Fork 139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Compiler] Support basic casting operations in VM #3767
Conversation
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:feature/compiler commit 56c8fbd Collapsed results for better readability
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I guess the core functions of the implementation, like ConvertAndBox
, Unbox
, BoxOptional
are "copies" of the one in the interpreter? Maybe we can refactor them (in a follow-up) so they only exist in one place so we can share the logic between interpreter and VM, and there is no chance their behaviour drifts in different directions?
Yes. It's not possible to refactor yet, because the values are different, and also interpreter's functions rely on I've added a item to our laundry-list for the next milestone, for adding those. |
…into supun/casting
8ceec6a
to
462e842
Compare
462e842
to
c76e75a
Compare
Like discussed, I split the different variants of casts in to separate instructions: c76e75a. Can you please have another look on the last commit? 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Looks great 👍
4e661a7
to
617be19
Compare
Opened #3771 to also add some test cases for the compilation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
Work towards #3742
Description
Adds basic support for type-casting in the VM.
Conversion for values based on type (e.g:
Int16
toInt32
, etc.) is not yet supported (left a TODO). It makes more sense to add those once the interpreter values are re-used.master
branchFiles changed
in the Github PR explorer