bytecodealliance / wasmtime

A fast and secure runtime for WebAssembly
https://wasmtime.dev/
Apache License 2.0
15.32k stars 1.29k forks source link

Cranelift: Signed SIMD vectors aren't allowed by IR parser #3276

Closed dheaton-arm closed 3 years ago

dheaton-arm commented 3 years ago

Various opcodes (such as SaddSat) only make sense if SIMD vectors can contain negative values, however the IR parser raises an error if a negative immediate is supplied, requiring explicitly u8 decimal immediates.

By supplying the 2's complement of the negative value in a SIMD vector, converting the values lanewise to signed values, and then performing a signed operation on the converted values, the correct results can be achieved (albeit again returned as the 2's complement, as unsigned values).

I would think given the existence of opcodes that rely on signed SIMD vectors, signed immediates should be allowed. Perhaps an unsigned variant (eg, u64x2) of each vector would make sense? These unsigned variants could behave as the current SIMD implementation does, but i64x2 could behave as a vector of signed values, instead.

bjorn3 commented 3 years ago

Adding u64x2 would require adding u64 too. Cranelift IR currently doesn't distinguish between unsigned and signed integers, only between unsigned and signed instructions, with many instructions working on both due to the way 2s-complement works. I would like it to stay this way.

I think it would be better to allow both unsigned and signed immediates just like for scalar operations.

afonso360 commented 3 years ago

We discussed this somewhat in #3059, not specifically for SIMD, but allowing a range of negative and positive integers based on the size of the type. I think it would also make sense to apply those same rules for the lane type in SIMD consts.

I agree with @bjorn3, keeping a single type, but allowing signed and unsigned immediates seems like the best way to go about this.

dheaton-arm commented 3 years ago

If the un/signed distinction occurs based on the instruction, then that makes sense.

dheaton-arm commented 3 years ago

Hi, sorry, after further testing, it turns out my description of this isn't entirely correct anyway. The problem here lies solely in i8x16 vectors, as the parser specifically checks for u8 immediates in vectors; if this is incorrect behaviour anyway, I'm happy to try fix that, but I wanted to check first if there was a specific reason for it. i16x8 and above do support signed values.

abrown commented 3 years ago

I think it is reasonable to parse i8x16 as lanes of i8--I can't recall precisely why u8 was used but it may have just been because it was available and i8 parsing was not. Adding i8 parsing makes sense to me.