Looking to help out with the interpreter and wanted to ask if anyone had any thoughts in terms of direction, such as what people are working on, or if anyone had any strong opinions on what I should start with? (I see that there's quite a few unimplemented instructions in step.rs
)
Hey, we are actively working on the interpreter right now, so help would be much appreciated!
Currently the work being done is with the intention of using this interpreter in the new fuzzer (tracked in #3050). Some recent work was in implementing i128 ops support (very early, we only support like 6 ops) and supporting jump tables.
Right now I'm working on implementing Heap and Stack support for the interpreter. Although we first need to support it in our test infrastructure.
I don't have any opinions on what we need to prioritize after heap and stack support, I think its mostly going to be implementing the rest of the CLIF opcodes. I know that i128 support is very incomplete, as well as SIMD ops support, regular arithmetic instructions are very under tested in our test suite, however they should be mostly implemented in the interpreter.
Alright, thanks; so working through some of the unimplemented!
s in step.rs
isn't a bad idea? (Just to check I've understood correctly)
No, not at all. Although some of them might be hard to get started with. Arithmetic ops such as UaddSat
and similar, should be fairly simple to implement and might be a good starting point!
Not sure if I'm missing something, but trying to test my implementation of UaddSat
using clif-util on an IR file, I'm not sure how to return the value from the function (as UaddSat
only supports SIMD vectors, which IR seems to use i32x4
(eg) notation for, but in the function signature it tries interpreting that as hex?).
Oops, didn't realize that UaddSat
was SIMD only.
Anyway, you can have a look at the existing test suite for SIMD, simd-arithmetic.clif
should have pretty much what you need.
but in the function signature it tries interpreting that as hex?
That sound's weird, do you have an example test case to share?
Thanks for the link to that test suite; turns out it was a syntax issue but the error attributed it to the wrong line (1) - I'd been declaring the SIMD vectors wrong (if you don't specify all the lanes and only put one number it complains about expecting hexadecimal on line 1).
For instructions like Shuffle
, how are the immediate bytes retrieved from step
? arg(0)
yields StepError::UnknownValue
and imm()
fails because it tries to unwrap a None
.
Also, in the IR the cls
instruction doesn't specify any specific integer sizes, but on the AArch64
backend it sign-extends anything less than 32-bit to 32-bit (though this isn't currently implemented at all for x86_64
). Should the interpreter mimic this behaviour and sign-extend to 32-bit before counting?
Hey, sorry I missed the earlier message.
For instructions like Shuffle, how are the immediate bytes retrieved from step? arg(0) yields StepError::UnknownValue and imm() fails because it tries to unwrap a None.
It looks like we store that outside of the InstructionData struct. So we would have to get the current DFG and query it for the mask. There is an example of this in the machinst/lower.rs file. So extrapolating from that, we would need something like the following:
if let InstructionData::Shuffle { mask, .. } = inst {
state.get_current_function().dfg.immediates.get(mask)
}
Also, in the IR the cls instruction doesn't specify any specific integer sizes, but on the AArch64 backend it sign-extends anything less than 32-bit to 32-bit (though this isn't currently implemented at all for x86_64). Should the interpreter mimic this behaviour and sign-extend to 32-bit before counting?
If i'm looking at this correctly, it looks like it zero extends it to 32 bits. But that doesn't look like it would work for i16 and i8 types...
Building this test case:
function %cls(i8) -> i8 {
block0(v0: i8):
v1 = cls.i8 v0
return v1
}
; run: %cls(0x00) == 7 ; We get 31 as a result here, which is wrong
; run: %cls(0x80) == 0 ; we get 23 here
; run: %cls(0xC0) == 1 ; we get 23 here
Yeah, it looks like the aarch64 backend has bugs here.
The correct thing to do in the interpreter is to just count the bits, and ignore this bug.
Filed a bug report for this case: https://github.com/bytecodealliance/wasmtime/issues/3248
The docs for umulhi seem contradictory as the description says it does not support vector types but the inputs / outputs specify it does. I'm assuming the text is correct and I should ignore the inputs / outputs in this case?
Yes, they are definitly contradictory
I would go with the Inputs/Outputs, it looks like the aarch64 backend has this implemented for vector types but the x64 backend does not
It looks like we don't use these instructions in code_translator.rs
We may also want to revisit this in the CLIF instruction cleanup. cc: @Andrew Brown, @Chris Fallin
For sqmul_round_sat
, the docs specify either scalar or vector values are fine, but the parser rejects i16
and i32
as "not a valid typevar". Not sure whether it's the docs or the parser that's wrong...
It looks like that instruction was added for simd. But I'm not sure if there are plans to use it for scalar values...
I would add it for SIMD for now, and we can discuss if we want to keep it around for scalar values in the PR. Implementing for scalar shouldn't be too much work after that.
+1, it's good to implement at least SIMD (or whatever is at parity with what machine backends accept, in general), and we can use this as an opportunity to document where the 'holes' in accepted types are. I'd like for a future where we're as polymorphic as possible -- it's difficult from the CLIF-producer PoV to work around missing corners -- and only reject types where it really doesn't make sense. But that's a long-term goal and not something to worry about today :-)
I added the instruction, and I think I was aiming for the polymorphic as possible future, as @Chris Fallin said :smile:, even though Wasm required only the SIMD variant (and only 16-bit elements on top of that).
It looks like I might have made a mistake when defining the type variable - I thought that scalars were included by default.
Last updated: Dec 23 2024 at 12:05 UTC