Skip to content

Begin validating some SPIR-V extensions/capabilities (esp. IntN/FloatN) on SPIR-T. #332

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

Merged
merged 4 commits into from
Jul 12, 2025

Conversation

eddyb
Copy link
Collaborator

@eddyb eddyb commented Jul 12, 2025

(Each commit should be reviewed separately)


Quick preview (from the diff of the last commit):

BeforeAfter
error: missing required capabilities for types
  |
  = note: `u8` type used without `OpCapability Int8`
  = note: `u16` type used without `OpCapability Int16`
error: `u8` type used without `OpCapability Int8`
   |
note: used from within Fragment entry-point `main`
  --> $DIR/scalars.rs:25:5
   |
25 |     #[spirv(flat)] in_u8: u8,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^
 
error: `u16` type used without `OpCapability Int16`
   |
note: used from within Fragment entry-point `main`
  --> $DIR/scalars.rs:26:5
   |
26 |     #[spirv(flat)] in_u16: u16,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^

This is the start of the strategy I was describing back in:

What made me go back to this was looking at this PR:

And realizing that the check_type_capabilities it added was closer to the SPIR-T-based error reporting (which isn't really used yet, unless a SPIR-T pass emits a diagnostic, or if RUSTGPU_CODEGEN_ARGS=--no-early-report-zombies is used, which turns off the old zombie reporting logic and lets them be treated like SPIR-T diagnostics).

However, there's two issues with the kind of ad-hoc reporting that check_type_capabilities does:

  • such errors lack any source information, so users may be helpless in tracking them down
    • both the zombie reporter, and the newer SPIR-T diagnostics one, give a full "backtrace"
    • FWIW, spirv-val errors are just as bad (another reason to prefer SPIR-T-based validation)
  • placing it before SPIR-T legalization passes can result in false positives
    • not a concern on main, but my local branch runs into e.g. requiring Int8 because of pointer arithmetic that qptr completely rewrites away
    • also, there's not much stopping us from using SPIR-T passes to legalize patterns like
      Capability is required unexpectedly for constant cast #300
      (I haven't tested it yet but the reduce pass w/ const-folding might already do that!)

In the interest of completeness/uniformity, I did add a few more SPIR-V extension/capability checks, which may impact some users, so this may need a bit more testing.
(Though, worst case, it will force you to add an extension/capability that is redundant with another, or with a SPIR-V version that is recent enough to have already incorporated an extension, or part thereof, etc.)

@eddyb eddyb force-pushed the dezombiefication branch from 39069bd to 3709a5e Compare July 12, 2025 05:17
@eddyb eddyb enabled auto-merge July 12, 2025 06:32
Copy link
Collaborator

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me, though I am still new to the spir-t side. It would be nice if spirt used something like salsa or differential dataflow or https://github.com/feldera/feldera/tree/main/crates/dbsp to calc on the fly vs at the end.

@eddyb eddyb added this pull request to the merge queue Jul 12, 2025
@eddyb
Copy link
Collaborator Author

eddyb commented Jul 12, 2025

It would be nice if spirt used something like salsa or differential dataflow or https://github.com/feldera/feldera/tree/main/crates/dbsp to calc on the fly vs at the end.

At this point I doubt that anything like incremental recomputation would make sense with the existing SPIR-T library - the lessons learned from it would probably need to be transplanted onto something new.

(It's possible I didn't get what you were referencing - maybe the ugly Transformed stuff? - but this is a pretty simple pass, all things considered, and the caching boilerplate hasn't been a priority because, esp. in the post-disaggregate future, there just aren't many reasons for non-shallow SPIR-T types/consts to exist)

Merged via the queue into Rust-GPU:main with commit e56cc4d Jul 12, 2025
13 checks passed
@eddyb eddyb deleted the dezombiefication branch July 12, 2025 14:58
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