Skip type checks on tables that don't need it #8172
Merged
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
Have the same function with slightly different variations to compare codegen between the possible strategies.
This commit implements an optimization to skip type checks in `call_indirect` for tables that don't require it. With the function-references proposal it's possible to have tables of a single type of function as opposed to today's default `funcref` which is a heterogenous set of functions. In this situation it's possible that a `call_indirect`'s type tag matches the type tag of a `table`-of-typed-`funcref`-values, meaning that it's impossible for the type check to fail. The type check of a function pointer in `call_indirect` is refactored here to take the table's type into account. Various things are shuffled around to ensure that the right traps still show up in the right places but the important part is that, when possible, the type check is omitted entirely.
github-actions
bot
added
cranelift
jameysharp
approved these changes
Mar 18, 2024
Co-authored-by: Jamey Sharp <jamey@minilop.net>
fitzgen
reviewed
Mar 18, 2024
Comment on lines
+1237
to
+1255
| // Otherwise if the types don't match then either (a) this is a | ||
| // null pointer or (b) it's a pointer with the wrong type. | ||
| // Figure out which and trap here. | ||
| // | ||
| // Note that the callee may be null in which case this load may | ||
| // trap. If so use the `IndirectCallToNull` trap code. | ||
| let mem_flags = ir::MemFlags::trusted().with_readonly(); | ||
| let callee_sig_id = self.builder.ins().load( | ||
| sig_id_type, | ||
| mem_flags.with_trap_code(Some(ir::TrapCode::IndirectCallToNull)), | ||
| funcref_ptr, | ||
| i32::from(self.env.offsets.ptr.vm_func_ref_type_index()), | ||
| ); | ||
| // If it's possible to have a null here then try to load the | ||
| // type information. If that fails due to the function being a | ||
| // null pointer, then this was a call to null. Otherwise if it | ||
| // succeeds then we know it won't match, so trap anyway. | ||
| if table.table.wasm_ty.nullable { | ||
| let mem_flags = ir::MemFlags::trusted().with_readonly(); | ||
| self.builder.ins().load( | ||
| sig_id_type, | ||
| mem_flags.with_trap_code(Some(ir::TrapCode::IndirectCallToNull)), | ||
| funcref_ptr, | ||
| i32::from(self.env.offsets.ptr.vm_func_ref_type_index()), | ||
| ); | ||
| } | ||
| self.builder.ins().trap(ir::TrapCode::BadSignature); | ||
| return CheckIndirectCallTypeSignature::StaticTrap; |
There was a problem hiding this comment.
FWIW, when the GC proposal is enabled, the static immediate type on the call_indirect will be allowed to be a subtype of the actual function's type.
We don't implement this yet, but will need to.
https://webassembly.github.io/gc/core/exec/instructions.html#exec-call-indirect
There was a problem hiding this comment.
Oh good point! I keep consistently forgetting that contra/covariance applies to params/results...
This makes me think we should remove the PartialEq implementation for types in wasmtime-types like we did in the embedder API as well
There was a problem hiding this comment.
Yeah that wouldn't be a bad idea.
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.
This commit implements an optimization to skip type checks in
call_indirectfor tables that don't require it. With thefunction-references proposal it's possible to have tables of a single
type of function as opposed to today's default
funcrefwhich is aheterogenous set of functions. In this situation it's possible that a
call_indirect's type tag matches the type tag of atable-of-typed-funcref-values, meaning that it's impossible for thetype check to fail.
The type check of a function pointer in
call_indirectis refactoredhere to take the table's type into account. Various things are shuffled
around to ensure that the right traps still show up in the right places
but the important part is that, when possible, the type check is omitted
entirely.