Stream: git-wasmtime

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


view this post on Zulip 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 ¯\\\_(ツ)\_\/¯.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2023 at 16:30):

afonso360 requested jameysharp for a review on PR #5971.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2023 at 17:53):

jameysharp submitted PR review.

view this post on Zulip 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 {

view this post on Zulip 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.)

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2023 at 17:53):

jameysharp submitted PR review.

view this post on Zulip 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...

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2023 at 19:09):

afonso360 submitted PR review.

view this post on Zulip 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!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2023 at 11:15):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2023 at 11:15):

afonso360 submitted PR review.

view this post on Zulip 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 {

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2023 at 22:10):

jameysharp submitted PR review.

view this post on Zulip 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:

view this post on Zulip Wasmtime GitHub notifications bot (Mar 11 2023 at 11:59):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 11 2023 at 12:00):

afonso360 has enabled auto merge for PR #5971.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 11 2023 at 12:58):

afonso360 merged PR #5971.


Last updated: Dec 23 2024 at 12:05 UTC