Conversation
| } | ||
| case ExtendSInt32: shouldBeEqual(curr->value->type, i32, curr, "extend type must be correct"); break; | ||
| case ExtendUInt32: shouldBeEqual(curr->value->type, i32, curr, "extend type must be correct"); break; | ||
| case ExtendSInt32: |
There was a problem hiding this comment.
Perhaps the original extend instructions should be renamed now?
There was a problem hiding this comment.
Maybe. Actually a lot of instructions probably should be renamed, and probably along with the binary format opcodes, it's kind of a mess (a lot of the opcode names predate the current spec scheme). I almost renamed the extension instructions here but decided that's not a great idea to mix it in with a functionality PR.
There was a problem hiding this comment.
Makes sense, probably better for a cleanup PR.
There was a problem hiding this comment.
Agreed, let's do a bulk renaming eventually to the spec names, but not here.
binji
left a comment
binji
left a comment
There was a problem hiding this comment.
lgtm, but did you want to implement in interpreter as well?
Not in this PR. I actually haven't implemented any of them in the interpreter yet, because my focus has been plumbing things through so we can test in engines. Although this one would be much simpler than the ones requiring shared memory. |
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Please add a TODO in passes/OptimizeInstructions.cpp that we can use these new instructions for code shrinking purposes. We have a bunch of code there to detect sign extends as well as create them etc., and this would be more compact (one instruction instead of one plus a constant).
src/wasm.h
Outdated
| DemoteFloat64, // f64 to f32 | ||
| ReinterpretInt32, ReinterpretInt64, // reinterpret bits to float | ||
| // The following sign-extention operators go along with wasm atomics support. | ||
| // Extend signed subword-sized integer. This differs from e.g. ExtendSInt64 |
| // The following sign-extention operators go along with wasm atomics support. | ||
| // Extend signed subword-sized integer. This differs from e.g. ExtendSInt64 | ||
| // because the input integer is in an i64 value insetad of an i32 value. | ||
| ExtendS8Int32, ExtendS16Int32, ExtendS8Int64, ExtendS16Int64, ExtendS32Int64, |
There was a problem hiding this comment.
maybe names should be ToInt32 etc., to match the trunc and converts? but maybe not since there are others too...
There was a problem hiding this comment.
Yeah, the Trunc* and Convert* opcode names use ToType but Promote, Demote, Reinterpret, and Extend don't. So... ¯\_(ツ)_/¯
| } | ||
| case ExtendSInt32: shouldBeEqual(curr->value->type, i32, curr, "extend type must be correct"); break; | ||
| case ExtendUInt32: shouldBeEqual(curr->value->type, i32, curr, "extend type must be correct"); break; | ||
| case ExtendSInt32: |
There was a problem hiding this comment.
Agreed, let's do a bulk renaming eventually to the spec names, but not here.
3 participants
These are not atomic operations, but are added with the atomic operations to keep from having to define atomic versions of all the sign-extending loads (an atomic zero-extending load + signext operation can be used instead).