-
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
Fix validating wasm stores of boolean vector results #3202
Fix validating wasm stores of boolean vector results #3202
Conversation
Previously cranelift's wasm code generator would emit a raw `store` instruction for all wasm types, regardless of what the cranelift operand type was. Cranelift's `store` instruction, however, isn't valid for boolean vector types. This commit fixes this issue by inserting a bitcast specifically for the store instruction if a boolean vector type is being stored, continuing to avoid the bitcast for all other vector types. Closes bytecodealliance#3099
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! You beat me to it...
// bitcast them to a vector type which is compatible with the store | ||
// instruction. | ||
if val_ty.is_vector() && val_ty.lane_type().is_bool() { | ||
val = builder.ins().raw_bitcast(I8X16, val); |
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.
Should we instead define that boolean vectors can be stored using store
directly and are stored as all-zero or all-one lanes?
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.
That is a good approach but I don't mind what @alexcrichton tried here because:
- vector translation already involves a lot of bitcasting so this doesn't make the problem worse; if we want to solve the bitcasting we should just solve all of it and close Too many raw_bitcasts in SIMD code #1147
- changing
load
andstore
to allow boolean vectors but not boolean scalars seems like it might cause errors somewhere; this fix would immediately fix a fuzz bug - I wouldn't mind settling the general issue about
load
s andstore
s with booleans once for all: I've seen mention recently of setting a memory representation for boolean scalars... If we settle it once for all (do we have an issue for this?) and document it then it might make sense to avoid the bitcast (if that's what is decided).
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.
I just created #3205 for this. Agreed that for now, the immediate fuzzbug fix is fine.
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.
Ok I'm gonna go ahead and merge this fix for now and defer to #3205 for future updates.
@abrown oh I don't want to step on any toes, I was mostly just curious myself to see how involved this might be (turns out not much) |
Previously cranelift's wasm code generator would emit a raw
store
instruction for all wasm types, regardless of what the cranelift operand
type was. Cranelift's
store
instruction, however, isn't valid forboolean vector types. This commit fixes this issue by inserting a
bitcast specifically for the store instruction if a boolean vector type
is being stored, continuing to avoid the bitcast for all other vector types.
Closes #3099