[move-compiler-v2, VM] add bytecode generator for lambdas #15485
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
⏱️ 1h 24m total CI duration on this PR
|
This was referenced Dec 4, 2024
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
brmataptos
force-pushed
the
11-23-vm_changes_merged_in
branch
from
1d8a057 to
d382ca0
Compare
brmataptos
force-pushed
the
12-01-add_bytecode_generator
branch
from
6f9ec9e to
0d44068
Compare
brmataptos
force-pushed
the
11-23-vm_changes_merged_in
branch
from
d382ca0 to
2fa8bde
Compare
brmataptos
force-pushed
the
12-01-add_bytecode_generator
branch
from
0d44068 to
8e869be
Compare
brmataptos
force-pushed
the
11-23-vm_changes_merged_in
branch
from
2fa8bde to
d7ded7a
Compare
brmataptos
force-pushed
the
12-01-add_bytecode_generator
branch
from
8e869be to
06110d7
Compare
brmataptos
force-pushed
the
11-23-vm_changes_merged_in
branch
from
d7ded7a to
844871b
Compare
brmataptos
force-pushed
the
12-01-add_bytecode_generator
branch
2 times, most recently
from
0982a66 to
128417e
Compare
brmataptos
force-pushed
the
11-23-vm_changes_merged_in
branch
from
fdd0ca9 to
e8107d7
Compare
brmataptos
force-pushed
the
12-01-add_bytecode_generator
branch
2 times, most recently
from
ba7ee0f to
dcd5597
Compare
brmataptos
requested review from
fEst1ck,
georgemitenkov,
rahxephon89,
runtian-zhou,
vineethk and
wrwg
brmataptos
force-pushed
the
12-01-add_bytecode_generator
branch
from
a177bcc to
f3e3fb3
Compare
… values based on generic functions not *store*, to match current MVP plans
- debug function value operations in `move-compiler-v2/src/file_format_generator/function_generator.rs` - complete codegen for function types in `module_generator` - add function `SignatureToken::is_subtype_of` which allows for subsetting of function abilities - carefully use subtype tests in `move-bytecode-verifier/src/type_safety.rs` for contexts where function types may show up - fix `lambda_lifter::gen_closure_function_name` to use alpha-numerics for symbols to avoid errors - add some tests for ENABLE_FUNCTION_VALUES feature gating using Move source code in `move_feature_gating.rs` - example `doable_func.lambda.exp` still shows an error due to imported public functions not being storable
brmataptos
force-pushed
the
12-01-add_bytecode_generator
branch
from
f3e3fb3 to
c92a8a3
Compare
brmataptos
changed the title
vineethk
reviewed
Dec 16, 2024
| Tok::LParen | Tok::Amp | Tok::AmpMut | Tok::Pipe | Tok::Identifier | ||
| ) | ||
| match (context.tokens.peek(), context.tokens.content()) { | ||
| (Tok::Identifier, "with") => false, |
There was a problem hiding this comment.
Is there a test that showcases a corresponding parser error?
| let global_access = ExpData::Call(global_id, Operation::Global(None), vec![ | ||
| zero_addr.into_exp() | ||
| ]); | ||
| let global_access = ExpData::Call( |
There was a problem hiding this comment.
Many of these changes seem to be only formatting changes that seem to revert back when I run the formatter. Might be good to be run the rust formatter before the PR to minimize such changes.
| @@ -2486,7 +2486,7 @@ pub enum Value { | |||
| Vector(Vec<Value>), | |||
| Tuple(Vec<Value>), | |||
| /// Represents a reference to a Move Function as a function value. | |||
| Function(ModuleId, FunId), | |||
| Function(ModuleId, FunId, Vec<Type>), | |||
There was a problem hiding this comment.
Add a brief note about the new field in the doc string above.
| .get_name() | ||
| .display(env.symbol_pool()) | ||
| .to_string(); | ||
| let name = format!("{}__lambda_{}", basename, self.lifted.len() + 1); |
There was a problem hiding this comment.
Why the switch from the previous basename$lambda${} to this?
Also, a brief comment on linear search below (and why it is restricted to up to 256) would be useful.
| @@ -197,6 +216,10 @@ pub enum Operation { | |||
| UnpackVariant(ModuleId, StructId, Symbol, Vec<Type>), | |||
| BorrowVariantField(ModuleId, StructId, Vec<Symbol>, Vec<Type>, usize), | |||
|
|
|||
| // Lambda | |||
There was a problem hiding this comment.
Could we say a bit here about the arguments for these operations?
| let arg = safe_unwrap!(verifier.stack.pop()); | ||
| if (type_actuals.is_empty() && &arg != parameter) | ||
| if (type_actuals.is_empty() && !arg.is_subtype_of(parameter)) |
There was a problem hiding this comment.
(1) It might be worth reasoning if the replacement of equality by is_subtype_of can result in exploitable increased verification cost, (2) Whether the new type representation, which is being optimized for equality, would have any unexpected blowup in time.
| // On top of the stack is the closure, pop it. | ||
| let closure_ty = safe_unwrap!(verifier.stack.pop()); | ||
| // Verify that the closure type matches the expected type | ||
| if &closure_ty != expected_ty { | ||
| if !closure_ty.is_subtype_of(expected_ty) { |
| let arg_ty = safe_unwrap!(verifier.stack.pop()); | ||
| if &arg_ty != param_ty { | ||
| return Err(verifier.error(StatusCode::CALL_TYPE_MISMATCH_ERROR, offset)); | ||
| if !arg_ty.is_subtype_of(param_ty) { |
| @@ -373,10 +366,13 @@ fn ld_function( | |||
| break; | |||
| } | |||
| } | |||
| if !type_actuals.is_empty() { | |||
There was a problem hiding this comment.
Add a brief note about generic functions not being storable for now.
| @@ -673,7 +662,7 @@ fn verify_instr( | |||
|
|
|||
| Bytecode::StLoc(idx) => { | |||
| let operand = safe_unwrap!(verifier.stack.pop()); | |||
| if &operand != verifier.local_at(*idx) { | |||
| if !operand.is_subtype_of(verifier.local_at(*idx)) { | |||
There was a problem hiding this comment.
We should perhaps measure the performance implications of this change? Should such changes also be hidden behind a feature flag?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Finish codegen for lambdas.
NoFunctionto disallow function types as generic parametersSignatureToken::is_subtype_ofto be more flexible about function type abilitiesAbilitySetconstantsFUNCTION_*to make it easier to be consistent.How Has This Been Tested?
Added a few more function tests (as noted above). Examined output of existing tests as well.
May need to remove some function type parameters used in tests now that we're removing it from MVP. A few tests that used to generate code are now just erroring out.
Key Areas to Review
What can we do about recognizing public functions in file_format.rs? (See Slack discussion.)
Double-check that no ungated functionality in VM is broken.
Type of Change
Which Components or Systems Does This Change Impact?