wasmtime / issue #6851 Add splat simplify opt for various... · git-wasmtime

This ended up being quite a long reply, sorry! I just wanted to note down the steps that I use to debug these sort of stuff, it might be useful in the future! (There's a TLDR at the bottom)

So, running the tests individually shows us what went wrong. These test suites contain a bunch of individual tests so there is more than one failure. Here's what I got:

running 1 test
test wast::Cranelift::spec::simd_splat_pooling ... FAILED

failures:

---- wast::Cranelift::spec::simd_splat_pooling stdout ----
thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', cranelift/codegen/src/isa/x64/lower/isle.rs:550:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at 'should be implemented in ISLE: inst = `v10 = sadd_sat.i8 v5, v7`, type = `Some(types::I8)`', cranelift/codegen/src/machinst/lower.rs:749:21
thread '<unnamed>' panicked at 'should be implemented in ISLE: inst = `v11 = sadd_sat.i16 v5, v7`, type = `Some(types::I16)`', cranelift/codegen/src/machinst/lower.rs:749:21
thread '<unnamed>' panicked at 'should be implemented in ISLE: inst = `v10 = usub_sat.i8 v5, v7`, type = `Some(types::I8)`', cranelift/codegen/src/machinst/lower.rs:749:21
thread '<unnamed>' panicked at 'should be implemented in ISLE: inst = `v11 = usub_sat.i16 v5, v7`, type = `Some(types::I16)`', cranelift/codegen/src/machinst/lower.rs:749:21
thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', cranelift/codegen/src/isa/x64/lower/isle.rs:550:21
thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', cranelift/codegen/src/isa/x64/lower/isle.rs:550:21

Starting with the easy ones: sadd_sat.{i8,i16} and usub_sat.{i8,i16} are not implemented in ISLE. That means that we haven't yet implemented a machine code lowering for them in the x64 backend (ISLE is just the framework that we use for this!). But more importantly when reading the docs it looks like they aren't supported for scalars!

This sometimes happens, we allow an operation for vectors but not for scalars. Usually because they don't make sense for scalars, I think in this case it's just that no one has felt the need to implement it yet.

I think we should be able to remove the optimizations for both of these opcodes, and the rest of the *_sat ones as well since they look like they only support vectors.

Now onto the hard one. I ran the testsuite individually with RUST_LOG=trace cargo run -- wast --disable-cache tests/spec_testsuite/simd_splat.wast and pulled one of the offending functions out of the log. Essentially I just looked for any function with sshr since I didn't expect there to be duplicates in these files.

function u0:22(i64 vmctx, i64, i32, i32) -> i8x16 fast {
block0(v0: i64, v1: i64, v2: i32, v3: i32):
    v5 = ireduce.i8 v2
    v6 = splat.i8x16 v5
    v7 = sshr v6, v3
    return v7
}

It's really neat that it matches our optimization pretty much exactly! After optimizations it looks like this:

block0(v0: i64, v1: i64, v2: i32, v3: i32):
    v5 = ireduce.i8 v2
    v8 = sshr v5, v3
    v9 = splat.i8x16 v8
    return v9

So, now that we have a small reproducible example of what is going wrong, lets debug it. My first step was compiling both the pre-optimization function and the post optimization function. And something immediately went wrong!

The optimized version compiles cleanly! Which is really weird, since I got that out of the optimizer from the first version. I haven't seen this before but I do have an idea of what might be going wrong.

We don't print the type of the sshr in the CLIF code. What I think might be happening is that we are saying to CLIF: Build me an sshr that takes an i8 and i32 and produces an i8x16 and things blow up later in the pipeline.

Changing the optimization source to explicitly request the lane_type fixes it! We are now requesting an i8 out of the sshr

(rule (simplify (sshr ty (splat ty x) y))
      (splat ty (sshr (lane_type ty) x y)))

This is a really weird issue, and it probably should have been caught earlier in the pipeline instead of letting it propagate all the way to the backend which really just confuses things.

This relied a lot on intuition and also knowledge of how cranelift works, so don't worry if it's hard to figure it out! We really should have had better error messages to help people out on these issues.

TLDR:
* We should remove the {u,s}{add,sub}_sat optimizations since these opcodes only exist for vectors but not for scalars.
* We should pass in the lane_type into the sshr operation, otherwise the compiler gets really confused. (this also applies to ushr/ishl/rotr/rotl)

Stream: git-wasmtime

Topic: wasmtime / issue #6851 Add splat simplify opt for various...

Wasmtime GitHub notifications bot (Aug 16 2023 at 05:44):

Subscribe to Label Action

Wasmtime GitHub notifications bot (Aug 17 2023 at 08:58):

Wasmtime GitHub notifications bot (Aug 17 2023 at 10:53):

Wasmtime GitHub notifications bot (Aug 17 2023 at 12:01):

Wasmtime GitHub notifications bot (Aug 17 2023 at 12:31):

Wasmtime GitHub notifications bot (Aug 18 2023 at 00:40):

Wasmtime GitHub notifications bot (Aug 18 2023 at 00:42):