Skip to content

Fix/implement checked_{add,sub,mul} and saturating_{add,sub} (using SPIR-V instructions were possible).#561

Draft
eddyb wants to merge 2 commits intomainfrom
eddyb/checked-saturating-spv
Draft

Fix/implement checked_{add,sub,mul} and saturating_{add,sub} (using SPIR-V instructions were possible).#561
eddyb wants to merge 2 commits intomainfrom
eddyb/checked-saturating-spv

Conversation

@eddyb
Copy link
Copy Markdown
Member

@eddyb eddyb commented Apr 13, 2026

After this PR:

  • signed checked_{add,sub} still have no direct SPIR-V equivalents
    • our custom implementation used to have bugs around e.g. 0 + 0 / 0 - 0
      (fixed in this PR, and verified via Alive2: broken vs correct)
  • unsigned checked_{add,sub} and signed/unsigned checked_mul map to SPIR-V instructions
    (OpIAddCarry, OpISubBorrow, OpSMulExtended/OpUMulExtended)
  • saturating_{add,sub} use an Intel extension instead of plainly erroring
    (OpIAddSatINTEL/OpISubSatINTEL)
    • in time, we might choose to emulate these instructions (later in the pipeline)

TODO: add tests for the edge cases, consider the need for OpUAddSatINTEL/OpUSubSatINTEL.

@nazar-pc
Copy link
Copy Markdown
Contributor

Could you also comment on #403? Seems directly applicable here.

Copy link
Copy Markdown
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.

The checked functions look good, the saturating ones emitting Intel-extension code can't really be used / tested anywhere but not like you've been able to do that in the first place either.

And then I read this:

TODO: add tests for the edge cases, consider the need for OpUAddSatINTEL/OpUSubSatINTEL.

And thought I'd make you a quick difftest between CPU and spirv to see if it actually resolved the issue at checked-saturating-spv-difftest, and you can see it fail:

    ┌────────┬─────────────────────────────┬──────────────────────────────────┐
    │ Offset │ math-checked-saturating-cpu │ math-checked-saturating-rust-gpu │
    ├────────┼─────────┬────────┬──────────┼──────────┬──────────┬────────────┤
    │        │   Hex   │  Dec   │  ASCII   │   Hex    │   Dec    │   ASCII    │
    ├────────┼─────────┼────────┼──────────┼──────────┼──────────┼────────────┤
    │ 0x0084 │    00   │    0   │   \0     │    01    │     1    │            │
    ├────────┼─────────┼────────┼──────────┼──────────┼──────────┼────────────┤
    │ 0x008c │    00   │    0   │   \0     │    01    │     1    │            │
    ├────────┼─────────┼────────┼──────────┼──────────┼──────────┼────────────┤
    │ 0x00b4 │    00   │    0   │   \0     │    01    │     1    │            │
    ├────────┼─────────┼────────┼──────────┼──────────┼──────────┼────────────┤
    │ 0x00bc │    00   │    0   │   \0     │    01    │     1    │            │
    └────────┴─────────┴────────┴──────────┴──────────┴──────────┴────────────┘

Whereas with this PR:

thread 'main' (564342) panicked at /home/firestar99/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/wgpu-27.0.1/src/backend/wgpu_core.rs:1058:
wgpu error: Validation Error

Caused by:
  In Device::create_shader_module, label = 'Compute Shader'
        
Shader 'Compute Shader' parsing error: UnsupportedInstruction(Function, IAddCarry)

naga doesn't support IAddCarry...

So what about using shader passthrough on our difftest? Well, it exists with -1 and no error... caused by a segfault... caused by shader passthrough requiring you to pass explicit PipelineLayout since wgpu's autogenerated one is just empty... which required refactoring the wgpu runner... and there's 3 different copies of a wgpu runner, thanks AI, so this may just be a rewrite of our difftest wgpu runner.

So... the difftest only works if rebased on #334 :D

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.

3 participants