wasmtime / issue #6855 cranelift: Run verifier after opti... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #6855 cranelift: Run verifier after opti...

Wasmtime GitHub notifications bot (Aug 17 2023 at 11:05):

afonso360 opened issue #6855:

:wave: Hey

Feature

We should run the verifier after all of the optimization passes during testing, or when running via clif-util.

Benefit

This would really improve the error messages for egraphs optimization authors.

Here's a recent example: https://github.com/bytecodealliance/wasmtime/pull/6851#issuecomment-1682067457 these error messages are provided by the backends, but that really confuses things since what is really happening is that we are producing invalid CLIF in the mid end.

Both of those examples would have really neat error messages if they were produced by the verifier.

Implementation

We probably don't want to enable this by default since it will slow down the compilation pipeline.

On of my ideas on how to implement this would be to add a new flag (i.e. run_verifier_after_opts) that would be disabled by default but we could enable in clif-util and in our test runner.

I don't know if we already have a do_extra_checks flag, but we can also maybe use that one.

Alternatives

Running this in the fuzzer is also an option but I think the costs might outweigh the benefits. I'm not sure.

We can also not do this, and write down somewhere that it's a good idea to recompile the optimized code manually. I have previously started doing that after encountering weird issues, and this time it worked out.

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

afonso360 edited issue #6855:

:wave: Hey

Feature

We should run the verifier after all of the optimization passes during testing, or when running via clif-util.

Benefit

This would really improve the error messages for egraphs optimization authors.

Here's a recent example: https://github.com/bytecodealliance/wasmtime/pull/6851#issuecomment-1682067457 these error messages are provided by the backends, but that really confuses things since what is really happening is that we are producing invalid CLIF in the mid end.

Both of those examples would have really neat error messages if they were produced by the verifier.

Implementation

We probably don't want to enable this by default since it will slow down the compilation pipeline.

On of my ideas on how to implement this would be to add a new flag (i.e. run_verifier_after_opts) that would be disabled by default but we could enable in clif-util and in our test runner.

I don't know if we already have a do_extra_checks flag, but we can also maybe use that one. Or we could even use enable_verifier, but I'm not sure about that.

Alternatives

Running this in the fuzzer is also an option but I think the costs might outweigh the benefits. I'm not sure.

We can also not do this, and write down somewhere that it's a good idea to recompile the optimized code manually. I have previously started doing that after encountering weird issues, and this time it worked out.

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

afonso360 edited issue #6855:

:wave: Hey

Feature

We should run the verifier after all of the optimization passes during testing, or when running via clif-util.

Benefit

This would really improve the error messages for egraphs optimization authors.

Here's a recent example: https://github.com/bytecodealliance/wasmtime/pull/6851#issuecomment-1682067457 these error messages are provided by the backends, but that really confuses things since what is really happening is that we are producing invalid CLIF in the mid end.

Both of those examples would have really neat error messages if they were produced by the verifier.

Implementation

We probably don't want to enable this by default since it will slow down the compilation pipeline.

On of my ideas on how to implement this would be to add a new flag (i.e. run_verifier_after_opts) that would be disabled by default but we could enable in clif-util and in our test runner.

I don't know if we already have a do_extra_checks flag, but we can also maybe use that one. Or we could maybe even use enable_verifier.

Alternatives

Running this in the fuzzer is also an option but I think the costs might outweigh the benefits. I'm not sure.

We can also not do this, and write down somewhere that it's a good idea to recompile the optimized code manually. I have previously started doing that after encountering weird issues, and this time it worked out.

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

afonso360 edited issue #6855:

:wave: Hey

Feature

We should run the verifier after all of the optimization passes during testing, or when running via clif-util.

Benefit

This would really improve the error messages for egraphs optimization authors.

Here's a recent example: https://github.com/bytecodealliance/wasmtime/pull/6851#issuecomment-1682067457 these error messages are provided by the backends, but that really confuses things since what is really happening is that we are producing invalid CLIF in the mid end.

Both of those examples would have really neat error messages if they were produced by the verifier.

Implementation

We probably don't want to enable this by default since it will slow down the compilation pipeline.

On of my ideas on how to implement this would be to add a new flag (i.e. run_verifier_after_opts) that would be disabled by default but we could enable in clif-util and in our test runner.

I don't know if we already have a do_extra_checks flag, but we can also maybe use that one. Or we could maybe even use enable_verifier itself.

Alternatives

Running this in the fuzzer is also an option but I think the costs might outweigh the benefits. I'm not sure.

We can also not do this, and write down somewhere that it's a good idea to recompile the optimized code manually. I have previously started doing that after encountering weird issues, and this time it worked out.

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

bjorn3 commented on issue #6855:

Isn't this just a missing self.verify_if(fisa) call at the end of egraph_pass? All other optimizations have this already.

Wasmtime GitHub notifications bot (Aug 17 2023 at 13:27):

afonso360 commented on issue #6855:

Oh yeah, If we're already running it for all of the other passes we might as well run it for this one as well!

Wasmtime GitHub notifications bot (Aug 18 2023 at 06:35):

gurry commented on issue #6855:

Replacing the Ok(()) in last line of this function: https://github.com/bytecodealliance/wasmtime/blob/e250334b8ebfba9359802ab7f61bdd7c6085d87a/cranelift/codegen/src/context.rs#L348-L367
with self.verify_if(fisa) should do the trick, right?

Wasmtime GitHub notifications bot (Aug 18 2023 at 06:39):

gurry edited a comment on issue #6855:

Replacing the Ok(()) in the last line of this function: https://github.com/bytecodealliance/wasmtime/blob/e250334b8ebfba9359802ab7f61bdd7c6085d87a/cranelift/codegen/src/context.rs#L348-L367
with self.verify_if(fisa) should do the trick, right?

Wasmtime GitHub notifications bot (Aug 18 2023 at 07:26):

gurry commented on issue #6855:

Making the above change causes the following error to be displayed:

FAIL filetests\filetests\egraph\simd-splat-simplify.clif: optimize

Caused by:
    function %sshr_splat_into_splat_sshr(i64, i64) -> i64x2 fast {
    block0(v0: i64, v1: i64):
        v4 = sshr v0, v1
    ;   ^~~~~~~~~~~~~~~~
    ; error: inst3 (v4 = sshr.i64x2 v0, v1): arg 0 (v0) has type i64, expected i64x2

        v5 = splat.i64x2 v4
        v6 -> v5
    ;   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    ; error: inst4 (v5 = splat.i64x2 v4): arg 0 (v4) has type i64x2, expected i64

        return v5
    }

    ; 2 verifier errors detected (see above). Compilation aborted.

1356 tests
Error: 1 failure
error: test failed, to rerun pass `--test filetests`

when cargo test is run against the following opt with a missing lane-type:

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

So looks like it works as expected.

Wasmtime GitHub notifications bot (Aug 18 2023 at 08:28):

afonso360 commented on issue #6855:

Yeah, That's pretty much it! Would you like to open a PR with that change?

Wasmtime GitHub notifications bot (Aug 18 2023 at 08:43):

gurry commented on issue #6855:

Yes, let me do that.

Wasmtime GitHub notifications bot (Aug 18 2023 at 09:56):

afonso360 closed issue #6855:

:wave: Hey

Feature

We should run the verifier after all of the optimization passes during testing, or when running via clif-util.

Benefit

This would really improve the error messages for egraphs optimization authors.

Here's a recent example: https://github.com/bytecodealliance/wasmtime/pull/6851#issuecomment-1682067457 these error messages are provided by the backends, but that really confuses things since what is really happening is that we are producing invalid CLIF in the mid end.

Both of those examples would have really neat error messages if they were produced by the verifier.

Implementation

We probably don't want to enable this by default since it will slow down the compilation pipeline.

On of my ideas on how to implement this would be to add a new flag (i.e. run_verifier_after_opts) that would be disabled by default but we could enable in clif-util and in our test runner.

I don't know if we already have a do_extra_checks flag, but we can also maybe use that one. Or we could maybe even use enable_verifier itself.

Alternatives

Running this in the fuzzer is also an option but I think the costs might outweigh the benefits. I'm not sure.

We can also not do this, and write down somewhere that it's a good idea to recompile the optimized code manually. I have previously started doing that after encountering weird issues, and this time it worked out.

Last updated: Nov 22 2024 at 16:03 UTC