Add 7 new difftests and push constants support #321
Conversation
|
I'd love to have these for testing my const folding in #317 |
|
CI Ubuntu reports these failures: On my machine I get these failures: Could this be float inaccuracies between vendors? |
|
Yeah, I think it is floats because they all passed locally after I took care of some existing float issues. I'll take a look and either round or omit. |
|
I added a This is a lot of code and I don't really expect you to review it...AI wrote a lot of it and I reviewed and tweaked. I figure we'll iterate on main as it is super self-contained. I can probably break it up if you want though! As an aside, I think some of these should be runtests, as I am not sure what value we get diffing with wgsl for all of them. BTW, if test times get long we can shard in CI as theoretically we could have 1 job for each test as they are unrelated. |
|
(note it is not set to automerge as I'll want to squash first) |
|
Ok, the windows issue is definitely a wgpu + naga (dxc) bug. |
|
I've pulled out the repo for the wgpu/naga bug: https://github.com/LegNeato/wgpu-workgroup-memory-bug |
There was a problem hiding this comment.
What I love about this PR:
- we print any detected changes into the logs, so you can easily see in CI the detected difference without having to boot up that platform
- epsilon for floating point
- the
ComputeShadertrait to make it easy to switch out shaders - Skip cause we'll likely need it in more places than this
I wonder if we should separate out the difftest improvements from the actual difftests in separate PRs?
The only major concern I have is that there's a lot of copy-paste between the different variants of a single test case. They usually only differ by the impl ComputeShader given as a arg. Doesn't have to be this PR, can be a followup, but I'd like to see those almost identical mains collapse. Make all variants of a single difftest a single crate. Some ideas around how that could be done:
- Features. The runner reads all features of the single difftest crate (via
cargo_metadata) and runs the test with one feature being active at a time. - Args. Read from some metadata toml file the variants you want and pass them in as args, via
struct Config. Then switching between shaders is just a match, not some#[cfg(...)]monster. Would require every variant to depend on wgpu, naga and ash, but we need to compile all of them anyway. - Metadata could be like github runners to allow cartesian product:
# skip wgsl and cpu variant
[variants]
shader = ["rust", "wgsl"]
runner = ["wgpu", "ash", "cpu"]- I'd perfer metadata + args.
| Self { | ||
| epsilon: Some(epsilon), | ||
| ..Default::default() | ||
| } |
There was a problem hiding this comment.
Default output_type is Raw, where epsilon doesn't make any sense. Rename it to with_f32(epsilon: f32) -> Self and adjust the current docs?
I'll look into how hard this is.
Yeah, I think once we get the kinks worked out I want to do stuff like compiletest where we just drop a .rs file with magic comments and everything else is taken care of. There is too much boiler plate right now, but I didn't want to get too fancy at the start. The reason why I started it this way is I wanted to make sure we could test things like 3rd party deps, setting values in |
|
It's probably just too little (did I mention that I love this diff? ❤️ ) |
|
@Firestar99 can you try the latest on your machine to confirm bumping the epsilon makes the tests pass? I tried to slice this up into more logical commits but it is painful. I'll try to at least clean the history up a bit. |
LegNeato
force-pushed
the
moardifftests
branch
2 times, most recently
from
1faa42f to
4bdf355
Compare
|
The larger epsilon did fix that test, everything passes now! |
|
Just FYI, difftest are not clippy checked in CI, fixed in #329 |
|
Apart from a rebase and #321 (comment), this would be ready to be merged, right? |
|
Yep, just been offline. Will try to get to it in the next couple days if no one beats me to it |
New tests: - bitwise_ops: bit manipulation operations - trig_ops: trigonometric functions - control_flow_complex: nested loops and complex control flow - vector_swizzle: vector component access and swizzling - memory_barriers: workgroup memory synchronization - vector_extract_insert: dynamic vector element access - push_constants: push constants in compute shaders Infrastructure changes: - Add WgpuComputeTestPushConstants for push constant support - Enable PUSH_CONSTANTS feature in wgpu when needed - Register all existing unregistered tests in workspace
- Add a round macro for cross-plat compat - Add epsilon-based floating point comparison - Add human-readable output for float/int data - Add test coverage for new stuff - Update documentation
Also don't show stats/counts when delaing with raw diffs
I wanted to increase coverage, so I worked with AI to bang out some tests and infra changes.
New tests:
Infrastructure changes: