-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Remove the initializer from a global's type information #6349
Remove the initializer from a global's type information #6349
Conversation
This commit removes the `Global::initializer` field to instead only store type information about the global rather than its initialization state. Instead now the initializer is stored separately from the type of the global, and only for defined globals. This removes the need in a few locations to synthesize dummy initializers.
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "cranelift", "cranelift:wasm", "wasmtime:api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
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.
This is great. There's one leftover bit I think you meant to delete and one thing maybe you can make more clear, but overall this makes perfect sense.
crates/runtime/src/instance.rs
Outdated
@@ -1015,7 +1015,7 @@ impl Instance { | |||
// Initialize the global before writing to it | |||
ptr::write(to, VMGlobalDefinition::new()); | |||
|
|||
match global.initializer { | |||
match module.global_initializers[def_index] { |
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.
The connection between indexes of global_initializers
and defined_global_index
seems a little subtle to me, but maybe that's just because I'm not that used to these idioms. I think defined_global_index
should just be subtracting off num_imported_globals
, right? Could you use Iterator::zip
to join the initializers to their definitions?
Feel free to leave this as is, but if you see an easy way to make this more obviously correct that'd be nice.
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.
Ah yes indeed this can be simplified! I've switched it to iterate over module.global_initializers
which is more direct and clearer.
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.
This commit removes the
Global::initializer
field to instead only store type information about the global rather than its initialization state. Instead now the initializer is stored separately from the type of the global, and only for defined globals. This removes the need in a few locations to synthesize dummy initializers.