-
Notifications
You must be signed in to change notification settings - Fork 42
[interpreter] Implement load lane instructions #428
Conversation
v128.load8_lane v128.load16_lane v128.load32_lane v128.load64_lane Introduce a new ast type, SimdLoadLane, since it takes a lane index immediate, on top of the usual memarg, and also pops a v128 off, in addition to the index. The exact binary opcodes for these instructions are not yet fixed, I've used the ones currently implement in V8 and LLVM/Binaryen, we can change those later. Also added a new test generation script, and the generated test files.
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.
Looks good, but my preference would be to avoid introducing a one-off type like simd_lane_memop and put the index separately. And likewise inline all the one-off helper functions for it.
rossberg
left a comment
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.
LGTM with some more nits.
interpreter/exec/eval.ml
Outdated
| | None -> assert false | ||
| | Some (pack_size, laneidx) -> Memory.load_simd_lane v128 pack_size mem addr offset ty laneidx | ||
| | None -> Memory.load_value mem addr offset ty | ||
| | Some (pack_size, simd_load) -> V128 (Memory.load_simd_packed pack_size simd_load mem addr offset ty) |
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.
Nit: line break?
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.
Thanks. Slight off-topic, would having some sort of formatter run on our input files be valuable? That would help codify and catch some of these stylistic issues.
I recognize that the big problem might be tons of merge conflicts when merging spec back into the proposals though...
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.
Yeah, unfortunately, with all the active forks, we cannot make structural changes to the sources without creating a lot of pain for a lot of people. That also prevents some refactoring in the interpreter that I'd like to do (like pulling symbol resolution out of the parser).
v128.load8_lane
v128.load16_lane
v128.load32_lane
v128.load64_lane
Introduce a new ast type, SimdLoadLane, since it takes a lane index
immediate, on top of the usual memarg, and also pops a v128 off, in
addition to the index.
The exact binary opcodes for these instructions are not yet fixed, I've
used the ones currently implement in V8 and LLVM/Binaryen, we can change
those later.
Also added a new test generation script, and the generated test files.