Skip to content

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

LegNeato
Copy link
Collaborator

@LegNeato LegNeato commented Jul 4, 2025

I wanted to increase coverage, so I worked with AI to bang out some tests and infra changes.

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

@Firestar99
Copy link
Member

I'd love to have these for testing my const folding in #317

@Firestar99
Copy link
Member

CI Ubuntu reports these failures:
difftests::lang::core::ops::math_ops
difftests::lang::core::ops::matrix_ops
difftests::lang::core::ops::vector_ops

On my machine I get these failures:
difftests::lang::core::ops::math_ops
difftests::lang::core::ops::matrix_ops

Could this be float inaccuracies between vendors?

@LegNeato
Copy link
Collaborator Author

LegNeato commented Jul 4, 2025

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.

@LegNeato LegNeato marked this pull request as draft July 5, 2025 16:44
@LegNeato
Copy link
Collaborator Author

LegNeato commented Jul 6, 2025

I added a compat_round! macro that handled most of the differences, but could not get all of them. So I added epsilon support when comparing. And then because I was debugging with python scripts I added better debug output when dealing with f32s. And because I did that I reworked all the output cases to match.

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.

@LegNeato LegNeato marked this pull request as ready for review July 6, 2025 15:45
@LegNeato
Copy link
Collaborator Author

LegNeato commented Jul 6, 2025

(note it is not set to automerge as I'll want to squash first)

@LegNeato
Copy link
Collaborator Author

LegNeato commented Jul 7, 2025

Ok, the windows issue is definitely a wgpu + naga (dxc) bug.

@LegNeato
Copy link
Collaborator Author

LegNeato commented Jul 8, 2025

I've pulled out the repo for the wgpu/naga bug: https://github.com/LegNeato/wgpu-workgroup-memory-bug

Copy link
Member

@Firestar99 Firestar99 left a 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()
}
Copy link
Member

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],
Copy link
Member

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.

Copy link
Member

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

@LegNeato
Copy link
Collaborator Author

LegNeato commented Jul 9, 2025

I wonder if we should separate out the difftest improvements from the actual difftests in separate PRs?

I'll look into how hard this is.

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:

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 Cargo.toml, using SpirvBuilder from build.rs, etc. When I thought about all the possibilities I wanted to support, I realized I was just doing a giant DSL and it would be simpler to just use standard Rust 😄 . We can always use standard rust tools to reduce boilerplate like the include!() I used in one of the tests so the same shader code is used between ash and wgpu variants or macros and such.

@Firestar99
Copy link
Member

Firestar99 commented Jul 9, 2025

It's probably just too little ε but this fails on my machine (Ryzen 680M RDNA2 iGPU):

test difftests::lang::core::ops::matrix_ops ... thread 'main' panicked at tests/difftests/bin/src/main.rs:108:49:
Outputs differ:

matrix_ops (lang/core/ops/matrix_ops)
F32, ε=0.00001 • 8 differences • max: 1.526e-5 (0.00%)
─────────────────────────────────────────────────────────────────

▪ matrix_ops-rust
  → Raw:  /tmp/.tmpgPQ1J2 (6.2 KiB)
  → Text: /tmp/.tmpgPQ1J2.txt

▪ matrix_ops-wgsl
  → Raw:  /tmp/.tmpHEcalv (6.2 KiB)
  → Text: /tmp/.tmpHEcalv.txt

┌──────┬─────────────────┬─────────────────┬──────────┬───────┐
│  #   │ matrix_ops-rust │ matrix_ops-wgsl │  Δ abs   │  Δ %  │
├──────┼─────────────────┼─────────────────┼──────────┼───────┤
│ 185  │ -93.107101440   │ -93.107086182   │ 1.526e-5 │ 0.00% │
├──────┼─────────────────┼─────────────────┼──────────┼───────┤
│ 385  │ -93.107101440   │ -93.107086182   │ 1.526e-5 │ 0.00% │
├──────┼─────────────────┼─────────────────┼──────────┼───────┤
│ 585  │ -93.107101440   │ -93.107086182   │ 1.526e-5 │ 0.00% │
├──────┼─────────────────┼─────────────────┼──────────┼───────┤
│ 785  │ -93.107101440   │ -93.107086182   │ 1.526e-5 │ 0.00% │
├──────┼─────────────────┼─────────────────┼──────────┼───────┤
│ 985  │ -93.107101440   │ -93.107086182   │ 1.526e-5 │ 0.00% │
├──────┼─────────────────┼─────────────────┼──────────┼───────┤
│ 1185 │ -93.107101440   │ -93.107086182   │ 1.526e-5 │ 0.00% │
├──────┼─────────────────┼─────────────────┼──────────┼───────┤
│ 1385 │ -93.107101440   │ -93.107086182   │ 1.526e-5 │ 0.00% │
├──────┼─────────────────┼─────────────────┼──────────┼───────┤
│ 1585 │ -93.107101440   │ -93.107086182   │ 1.526e-5 │ 0.00% │
└──────┴─────────────────┴─────────────────┴──────────┴───────┘

(did I mention that I love this diff? ❤️ )

@LegNeato
Copy link
Collaborator Author

LegNeato commented Jul 9, 2025

@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 added 9 commits July 10, 2025 12:03
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
@Firestar99
Copy link
Member

The larger epsilon did fix that test, everything passes now!

@Firestar99
Copy link
Member

Just FYI, difftest are not clippy checked in CI, fixed in #329

@Firestar99
Copy link
Member

Firestar99 commented Jul 11, 2025

Apart from a rebase and #321 (comment), this would be ready to be merged, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants