Skip to content
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

[naga] change i32 arithmetic operations to use wrapping_ instead of checked_ #6835

Merged

Conversation

matthew-wong1
Copy link
Contributor

@matthew-wong1 matthew-wong1 commented Dec 29, 2024

Connections
Fixes #6023

Description
For arithmetic operations between two variables of type i32, if an overflow occurs, it is meant to wrap instead of throw an error according to the WGSL spec ("Expressions on concrete integer types that overflow produce a result that is modulo 2bitwidth")

Testing
Compiled the original program detailed in #6023 (and variations for other arithemtic operations) that threw the overflow error (now overflow error free)

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@matthew-wong1 matthew-wong1 marked this pull request as ready for review December 29, 2024 17:14
@matthew-wong1 matthew-wong1 requested a review from a team December 29, 2024 17:14
@matthew-wong1 matthew-wong1 changed the title [naga] change i32 multiplication to use wrapping_mul instead of checked_mul [naga] change i32 arithmetic operations to use wrapping_ instead of checked_ Dec 30, 2024
@matthew-wong1 matthew-wong1 force-pushed the update-i32-multiplication-to-wrap branch from fec4364 to b2dca02 Compare December 30, 2024 07:45
@sagudev
Copy link
Contributor

sagudev commented Dec 30, 2024

I will do CTS run in servo for this, but it will take some time as I need to set baseline expectations first.

sagudev added a commit to sagudev/servo that referenced this pull request Dec 30, 2024
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
sagudev added a commit to sagudev/servo that referenced this pull request Dec 30, 2024
{"fail_fast": false, "matrix": [{"name": "WebGPU CTS", "workflow": "linux", "wpt_layout": "2020", "profile": "production", "unit_tests": false, "bencher": false, "wpt_args": "_webgpu"}]}
@sagudev
Copy link
Contributor

sagudev commented Dec 30, 2024

CTS run looks good: https://github.com/sagudev/servo/actions/runs/12544331341/attempts/1#summary-34979468146

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Thanks!

@teoxoy teoxoy merged commit 826db5e into gfx-rs:trunk Jan 6, 2025
52 checks passed
@matthew-wong1 matthew-wong1 deleted the update-i32-multiplication-to-wrap branch January 6, 2025 16:02
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.

naga incorrectly throws multiplication operation overflowed error in expressions involving abstract numerics
3 participants