Stream: cranelift

Topic: interpreter

Damian Heaton (Aug 03 2021 at 11:13):

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)

Afonso Bordado (Aug 03 2021 at 11:27):

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.

Damian Heaton (Aug 03 2021 at 11:45):

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)

Afonso Bordado (Aug 03 2021 at 11:48):

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!

Damian Heaton (Aug 03 2021 at 16:25):

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?).

Afonso Bordado (Aug 03 2021 at 22:43):

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?

Damian Heaton (Aug 04 2021 at 09:58):

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).

Damian Heaton (Aug 24 2021 at 13:34):

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.

Damian Heaton (Aug 26 2021 at 10:55):

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?

Afonso Bordado (Aug 26 2021 at 11:26):

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.

Afonso Bordado (Aug 26 2021 at 12:09):

Filed a bug report for this case: https://github.com/bytecodealliance/wasmtime/issues/3248

Damian Heaton (Sep 01 2021 at 16:33):

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?

Afonso Bordado (Sep 01 2021 at 17:30):

Yes, they are definitly contradictory

Afonso Bordado (Sep 01 2021 at 17:31):

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

Afonso Bordado (Sep 01 2021 at 17:39):

It looks like we don't use these instructions in code_translator.rs

Afonso Bordado (Sep 01 2021 at 17:53):

We may also want to revisit this in the CLIF instruction cleanup. cc: @Andrew Brown, @Chris Fallin

Damian Heaton (Sep 08 2021 at 11:02):

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...

Afonso Bordado (Sep 08 2021 at 11:29):

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.

Chris Fallin (Sep 08 2021 at 16:23):

+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 :-)

Anton Kirilov (Sep 08 2021 at 16:40):

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).

Anton Kirilov (Sep 08 2021 at 16:50):

It looks like I might have made a mistake when defining the type variable - I thought that scalars were included by default.

Last updated: Oct 23 2024 at 20:03 UTC