cranelift: Choose memory trap code based on memflags #8134
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
cfallin
reviewed
Mar 14, 2024
github-actions
bot
added
cranelift
Ideally we'd allow the frontend to specify what trap code a memory access instruction can produce, but we don't have room to store that information. We do have a few bits to spare in `MemFlags`, though, so add a new `tabletrap` bit to indicate that the trap code should be TableOutOfBounds instead of the default HeapOutOfBounds. Nothing yet sets the `tabletrap` flag, so the TableOutOfBounds case is never hit, but I wanted to separate this change out from the cranelift-wasm changes which will use it. The original version of this patch reused the `table` flag instead of introducing a new `tabletrap` flag. But this usage changes the semantics of an instruction, while the `table` flag is meant to be a hint for alias analysis in the egraph optimization pass.
jameysharp
force-pushed
the
memflags-trapcode
branch
from
a4b8f24 to
992b804
Compare
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.
Ideally we'd allow the frontend to specify what trap code a memory access instruction can produce, but we don't have room to store that information.
Instead, I'm adding additional meaning to the
tableandheapmemflags. Besides being used in alias analysis, these flags now also indicate whether the instruction should produce a trap code of TableOutOfBounds or HeapOutOfBounds, respectively.In the Cranelift weekly meeting we discussed that it would be nice to assert that either
tableorheapis set when the trap-code is requested, but I don't know if all users of the instruction encodings (such as cg-clif or Winch) set those flags, so I've chosen to default to the old behavior if none of the alias-analysis flags are set.Currently, memory accesses with the
tableflag set always havenotrapset as well, so the TableOutOfBounds case is never hit, but I wanted to separate this change out from the cranelift-wasm changes which will use it.