wasmtime / PR #5971 fuzzgen: Add SIMD instructions suppor... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #5971 fuzzgen: Add SIMD instructions suppor...

Wasmtime GitHub notifications bot (Mar 09 2023 at 16:30):

afonso360 opened PR #5971 from fuzz-full-simd to main:

:wave: Hey,

This PR adds all SIMD instructions currently supported by the interpreter to the fuzzer. There is a surprising amount of missing coverage even with all of these ones (i.e. lots of work to do in the interpreter)!

I've also had to disable aligned loads/stores for > 8 bytes. It's really hard to do this without support from cranelift, and I think its probably best to go add support for that before re-enabling it here.

I gave this a bunch of fuzzing time, and it has stopped crashing on all arches. S390X was a little bit harder since I was only able to test it on QEMU and its quite a bit slower than native hardware though it ran about 12hours on 8 cores without crashing, so ¯\\\_(ツ)\_\/¯.

Wasmtime GitHub notifications bot (Mar 09 2023 at 16:30):

afonso360 requested jameysharp for a review on PR #5971.

Wasmtime GitHub notifications bot (Mar 09 2023 at 17:53):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 09 2023 at 17:53):

jameysharp created PR review comment:

What do you think of this style? (It may need help from rustfmt, I'm just typing things straight into this comment.)

        let unimplemented_cc = match (fgen.target_triple.architecture, cc) {
            // Some FloatCC's are not implemented on AArch64, see:
            // https://github.com/bytecodealliance/wasmtime/issues/4850
            (Architecture::Aarch64(_),
                FloatCC::OrderedNotEqual |
                FloatCC::UnorderedOrEqual |
                FloatCC::UnorderedOrLessThan |
                FloatCC::UnorderedOrLessThanOrEqual |
                FloatCC::UnorderedOrGreaterThan |
                FloatCC::UnorderedOrGreaterThanOrEqual) => true,
            (Architecture::X86_64, FloatCC::UnorderedOrEqual | FloatCC::OrderedNotEqual) if args[0].is_vector() => true,
            _ => false,
        };

        if unimplemented_cc {

Wasmtime GitHub notifications bot (Mar 09 2023 at 17:53):

jameysharp created PR review comment:

I think OpcodeConstraints::requires_typevar_operand is most of what we need to avoid having to update this table:
    let ctrl_type = if opcode.constraints().requires_typevar_operand() {
Also please update the comment above.

The rest of what we need is figuring out which operand is the typevar operand. It is _almost_ always operand 0, but it depends on the instruction format. For Ternary it's operand 1, and for TernaryImm8 it's operand 2. (See cranelift/codegen/meta/src/shared/formats.rs and look for calls to InstructionFormatBuilder::typevar_operand.)

Since insert_opcode is used for the Ternary format, this technically would look at the wrong typevar operand. Fortunately, all current Ternary-format opcodes have requires_typevar_operand=false, so we don't actually have to fix this today.

As future work, I think we should provide a method to get the index of the typevar operand. I think it should be on InstructionFormat. There's an InstructionData::typevar_operand method today that gets the Value from the appropriate operand of an instruction, but here we need the operand index. (We could replace the existing typevar_operand method by calling this new one and indexing into InstructionData::arguments, I think.)

Wasmtime GitHub notifications bot (Mar 09 2023 at 17:53):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 09 2023 at 17:53):

jameysharp created PR review comment:

I'm tempted to assert here that the provided opcode is Opcode::Shuffle, since this function will be called for any opcode that uses the Shuffle format. I mean, yes, there's only one such opcode today, but just in case...

Wasmtime GitHub notifications bot (Mar 09 2023 at 19:09):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Mar 09 2023 at 19:09):

afonso360 created PR review comment:

I think OpcodeConstraints::requires_typevar_operand is most of what we need to avoid having to update this table:

Oh that's really nice! I didn't know that existed.

As future work, I think we should provide a method to get the index of the typevar operand. I think it should be on InstructionFormat. There's an InstructionData::typevar_operand method today that gets the Value from the appropriate operand of an instruction, but here we need the operand index. (We could replace the existing typevar_operand method by calling this new one and indexing into InstructionData::arguments, I think.)

I was thinking about this in a slightly different way. With the new fancy constraint based generation we always know the correct ctrl_type, so we could update the inserter signature to also receive that. There are a bunch of inserters that currently do something like rets.first().unwrap() which we can cleanup.

Either way, this can definitely be improved!

Wasmtime GitHub notifications bot (Mar 10 2023 at 11:15):

afonso360 updated PR #5971 from fuzz-full-simd to main.

Wasmtime GitHub notifications bot (Mar 10 2023 at 11:15):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Mar 10 2023 at 11:15):

afonso360 created PR review comment:

I don't really mind that, but the rustfmt format for that makes it quite hard to parse for me, what do you think?

        let unimplemented_cc = match (fgen.target_triple.architecture, cc) {
            // Some FloatCC's are not implemented on AArch64, see:
            // https://github.com/bytecodealliance/wasmtime/issues/4850
            (
                Architecture::Aarch64(_),
                FloatCC::OrderedNotEqual
                | FloatCC::UnorderedOrEqual
                | FloatCC::UnorderedOrLessThan
                | FloatCC::UnorderedOrLessThanOrEqual
                | FloatCC::UnorderedOrGreaterThan
                | FloatCC::UnorderedOrGreaterThanOrEqual,
            ) => true,
            (Architecture::X86_64, FloatCC::UnorderedOrEqual | FloatCC::OrderedNotEqual)
                if args[0].is_vector() =>
            {
                true
            }
            _ => false,
        };
        if unimplemented_cc {

Wasmtime GitHub notifications bot (Mar 10 2023 at 22:10):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 10 2023 at 22:10):

jameysharp created PR review comment:

Hmm, yeah, that's... not ideal? It's just that I'd prefer to use match instead of slice::contains to let the compiler decide how to implement this check. I guess for me that preference slightly outweighs style preferences, but I'm willing to be persuaded otherwise.

Two things we could try which might improve the style:

Put each arch/cc combination in a separate match arm, e.g. (Architecture::Aarch64(_), FloatCC::OrderedNotEqual) => true instead of using or-patterns

Replace if x => true with => x

Wasmtime GitHub notifications bot (Mar 11 2023 at 11:59):

afonso360 updated PR #5971 from fuzz-full-simd to main.

Wasmtime GitHub notifications bot (Mar 11 2023 at 12:00):

afonso360 has enabled auto merge for PR #5971.

Wasmtime GitHub notifications bot (Mar 11 2023 at 12:58):

afonso360 merged PR #5971.

Last updated: Nov 22 2024 at 16:03 UTC