-
Notifications
You must be signed in to change notification settings - Fork 60
Add 7 new difftests and push constants support #321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ComputeShader
trait 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
|
||
fn run_compute( | ||
&self, | ||
spirv_bytes: &[u8], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this take spirv_bytes
as a &[u8]
and not something like impl ComputeShader
like wgpu does? Would need to adjust the ComputeShader
trait to support just returning like a spirv_bytes: &[u8]
. That would also remove a lot of the custom code in the single ash test we have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be fine deferring this to a followup as well
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. |
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
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? |
I wanted to increase coverage, so I worked with AI to bang out some tests and infra changes.
New tests:
Infrastructure changes: