afonso360 opened issue #4979:
:wave: Hey,
This was brought up yesterday in the cranelift meeting, and I'm filing this mostly so we don't forget.
Feature
Enable the cranelift
regalloc_checker
flag in our test suites.The concrete testsuites I'm thinking are the
filetests
testsuite for cranelift, and thespec_testsuite
for wasm. However if we can get others then that would be great too.Something that was brought up at the meeting was the performance of
regalloc_checker
, we should probably make sure that the testsuites don't become too slow.Benefit
This would have caught #4969 earlier and potentially other regalloc issues in the future.
Implementation
I'm most familiar with
filetests
and at least in thecompile
andrun
tests we can "clone" the flags requested by the tests and force enableregalloc_checker
. I'm not too sure aboutspec_testsuite
but maybe something similar would work too?Alternatives
Make it a default flag
@cfallin mentioned this yesterday with the additional note that the regalloc checker was never built for speed so it would probably have a somewhat large downside. This matches what I've seen when trying to fuzz it.
Run it only during fuzzing
I played around with this in the
cranelift-fuzzgen
fuzzer, and the preliminary results aren't great.In the sample run that I usually do (~90k inputs), with regalloc checker we run at 10 execs / sec and without we get 26 execs / sec. The other alternative that I've been thinking is running with regalloc checker only x% of the time although that is still a WIP.
cc: @cfallin @jameysharp @uweigand
afonso360 edited issue #4979:
:wave: Hey,
This was brought up yesterday in the cranelift meeting, and I'm filing this mostly so we don't forget.
Feature
Enable the cranelift
regalloc_checker
flag in our test suites.The concrete testsuites I'm thinking are the
filetests
testsuite for cranelift, and thespec_testsuite
for wasm. However if we can get others then that would be great too.Benefit
This would have caught #4969 earlier and potentially other regalloc issues in the future.
Implementation
I'm most familiar with
filetests
and at least in thecompile
andrun
tests we can "clone" the flags requested by the tests and force enableregalloc_checker
. I'm not too sure aboutspec_testsuite
but maybe something similar would work too?Something that was brought up at the meeting was the performance of
regalloc_checker
, we should probably make sure that the testsuites don't become too slow.Alternatives
Make it a default flag
@cfallin mentioned this yesterday with the additional note that the regalloc checker was never built for speed so it would probably have a somewhat large downside. This matches what I've seen when trying to fuzz it.
Run it only during fuzzing
I played around with this in the
cranelift-fuzzgen
fuzzer, and the preliminary results aren't great.In the sample run that I usually do (~90k inputs), with regalloc checker we run at 10 execs / sec and without we get 26 execs / sec. The other alternative that I've been thinking is running with regalloc checker only x% of the time although that is still a WIP.
cc: @cfallin @jameysharp @uweigand
cfallin commented on issue #4979:
This is definitely interesting to consider!
One thing that has occurred to me since our discussion is that turning the checker on in tests may have somewhat less value than in fuzzing (as long as fuzzing has sufficient coverage to touch every Wasm or CLIF opcode that the tests do). The reason is that regalloc errors tend to become more interesting as multiple instructions, or nontrivial control flow, interact. If we trivially miss an instruction input or output constraint, that will immediately cause tests to fail. But if, for example, we accidentally clobber some other register, we're more likely to see a failure if there is enough register pressure in the function that something else is also using that register.
As well, enabling the checker for the main testsuites increases the critical path length for CI, while fuzzing is a separate workflow off to the side.
So I suspect that "turn it on sometimes for some compilation fuzz targets" is our best option. I'm interested in what others think of this!
Last updated: Dec 23 2024 at 12:05 UTC