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 inclif-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.
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 inclif-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 useenable_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.
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 inclif-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 useenable_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.
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 inclif-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 useenable_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.
bjorn3 commented on issue #6855:
Isn't this just a missing
self.verify_if(fisa)
call at the end ofegraph_pass
? All other optimizations have this already.
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!
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
withself.verify_if(fisa)
should do the trick, right?
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
withself.verify_if(fisa)
should do the trick, right?
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 missinglane-type
:(rule (simplify (sshr ty (splat ty x) y)) (splat ty (sshr ty x y)))
So looks like it works as expected.
afonso360 commented on issue #6855:
Yeah, That's pretty much it! Would you like to open a PR with that change?
gurry commented on issue #6855:
Yes, let me do that.
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 inclif-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 useenable_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: Dec 23 2024 at 12:05 UTC