Conversation
|
That's awesome! Generally, I'm very interested in merging this and on first glance the code looks fine so far. (Probably a few small nitpicks here and there.) I just feel that I should read through the wasm simd spec before doing a full review and it might be a few days before I get to that. |
| .then_ignore(just("i64")) | ||
| .map(|n| Token::Int64(n as i64)); | ||
|
|
||
| let int128 = integer |
There was a problem hiding this comment.
integer currently parses to an i64. So this won't be able to cover the whole 128 bit range. Maybe it's enough to update integer to parse into an i128 instead.
There was a problem hiding this comment.
This seems to have worked! I also added i128 support to data sections for consistency even though I don't know why you wouldn't just use smaller units instead. I think I've managed to be consistent enough with using I128 to describe the literal type and V128 to describe the WASM type but I'm not 100% sure I got this right.
src/typecheck.rs
Outdated
| return type_mismatch(Some(I32), &span, param.type_, ¶m.span, context.sources); | ||
| } | ||
| let lane = param.const_i32(); | ||
| if lane < 0 || lane > 15 { |
There was a problem hiding this comment.
This definitely needs to check for the correct range for this specific instruction. The emit phase will most likely just panic when encountering an out-of-range lane.
But let me read the spec first, so that I can give a suggestion on how to best implement that.
There was a problem hiding this comment.
Ahh, okay, that makes sense. It looked like there was a fairly simple way to add these checks so I've tried to do that, but I'm certainly open to alternatives!
|
Great! I'm glad to hear this seems to be in scope. No rush on the review - happy to pick this back up whenever you get a chance to look at the spec. |
Also change the way lane widths are handled - it turns out that shuffle takes lane arguments up to 31, so it's necessary to store maximum lane index explicitly instead of deriving it from lane width.
|
Ok, I squeezed shuffle in, but that should be it, I think - I won't scope creep this any further except by request. :) |
|
Process request: if you do end up looking at this and wanting me to do something, can you ping me on Discord? luchak#7452, and I think we have a couple of servers in common. My Github notifications are just a bit of a mess. Similarly for exoticorn/microw8#4 . If not, no worries, I'm mostly focused on work and on non-sizecoding projects for the moment, I just wanted to make sure I wouldn't be dropping the ball here. |
|
It's been a while, but I just wanted to say that I'd be really excited to see this PR land if you ever have the time to review it! |
I present: a silky-smooth yak. I've tried to add support for the v128 type and WASM SIMD intrinsics.
Major warning: I have never written Rust before.
The biggest challenge I encountered was supporting instructions that take lane parameters. Following the way load and store instructions are treated now, I've added special handling for two new kinds of instructions:
MemLaneInstruction(loads and stores) andLaneInstruction(extract and replace lane). I'm not happy with my naming or the very copy/paste way that these new formats are handled. I'm sure something much better could happen here but I'm not comfortable enough with either Rust or the existing codebase to be sure what that would be.In addition to the v128 type and SIMD intrinsics, there's also a new i128 literal type, handled (hopefully) analogously to existing i64 literals.
I have done rudimentary spot checks of v128 constants, non-lane/non-memory SIMD operations, lane loads, lane stores, lane extractions, and lane replacements. At least one or two representative intrinsics in each of these classes seem to function correctly. That's been about the extent of my testing, however.
A few other warnings / deficiencies:
Formats for the new classes of instructions are:
And here is a quick and dirty smoke test:
Let me know if this is something you'd potentially be interested in merging, and if so what you'd need to make that happen.