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.

Sign up for GitHub

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

Jump to bottom

Add parsing support for un/pack4xI/U8 #5424

Merged
merged 2 commits into from
May 14, 2024
Merged

Add parsing support for un/pack4xI/U8 #5424

merged 2 commits into from
May 14, 2024

Conversation

VlaDexa
Copy link
Contributor

@VlaDexa VlaDexa commented Mar 22, 2024 •
edited
Loading

Connections

Description
WGSL spec defined new builtin functions. This PR tries to implement them

Current writer impls:

  • SPIR-V
  • HLSL
  • GLSL
  • MSL
  • WGSL

Testing
I think I added the necessary tests to naga/tests/in/bits.wgsl.

Checklist

  • Run cargo fmt.
  • 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.

@VlaDexa
Copy link
Contributor Author

VlaDexa commented Mar 22, 2024

I have no idea how to use those magic words that make the spir-v backend, so I really would appreciate some help here

@teoxoy
Copy link
Member

teoxoy commented Mar 28, 2024

I would look at some of the existing polyfills. For ex.:

wgpu/naga/src/back/spv/block.rs

Line 734 in c613fbb

Mf::Clamp => match arg_scalar_kind {

wgpu/naga/src/back/spv/block.rs

Line 987 in c613fbb

Mf::CountLeadingZeros => {

@VlaDexa VlaDexa force-pushed the un/packInt branch 3 times, most recently from 80aec2c to 89a8b16 Compare April 2, 2024 18:05
@VlaDexa VlaDexa marked this pull request as ready for review April 2, 2024 18:06
@VlaDexa VlaDexa requested a review from a team as a code owner April 2, 2024 18:06
@VlaDexa
Copy link
Contributor Author

VlaDexa commented Apr 8, 2024

@teoxoy Hey, it's been a week since this pr has been finished. Do I need to signal someone that it's ready ready or just wait for it to be reviewed, squashed and merged?

@teoxoy
Copy link
Member

teoxoy commented Apr 9, 2024

It's on my radar, there have been other PRs that need/needed attention as well.
I should get to it these coming days.

@teoxoy
Copy link
Member

teoxoy commented Apr 15, 2024

I squashed all the commits, rebased and extracted the match refactor to make it easier to review.

teoxoy
teoxoy reviewed Apr 15, 2024
View reviewed changes
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.

Since the code for the MSL, GLSL and HLSL backends is the same, we should try to put it in the back module instead.

naga/src/back/glsl/mod.rs Show resolved Hide resolved
naga/src/back/spv/block.rs Show resolved Hide resolved
naga/src/back/spv/block.rs Outdated Show resolved Hide resolved
naga/src/back/spv/block.rs Outdated Show resolved Hide resolved
@VlaDexa VlaDexa force-pushed the un/packInt branch 2 times, most recently from 5a2c551 to 7d48c17 Compare April 17, 2024 15:45
@teoxoy
Copy link
Member

teoxoy commented Apr 19, 2024

It would be great if you could put together some execution tests that make sure the polyfills work properly.

There are some existing ones at this path: https://github.com/gfx-rs/wgpu/tree/54740d5982fa5ae004eee95dd6c037356ce9ea17/tests/tests/shader.

@VlaDexa VlaDexa requested a review from a team as a code owner May 3, 2024 21:45
@VlaDexa VlaDexa force-pushed the un/packInt branch 2 times, most recently from a10a80c to 85f1046 Compare May 4, 2024 00:37
cwfitzgerald
cwfitzgerald requested changes May 9, 2024
View reviewed changes
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Small nits, but wgpu/test side looks good, by and large!

Sorry for the delay on the naga review, all the naga reviewers are pretty busy right now.

(Reminder: please re-request a review from me once the changes are addressed to make sure I see it!)

tests/tests/shader/data_builtins.rs Outdated Show resolved Hide resolved
tests/tests/shader/data_builtins.rs Outdated Show resolved Hide resolved
tests/tests/shader/data_builtins.rs Outdated Show resolved Hide resolved
@VlaDexa VlaDexa requested a review from cwfitzgerald May 10, 2024 14:31
cwfitzgerald
cwfitzgerald approved these changes May 11, 2024
View reviewed changes
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Approved from wgpu side :)

@cwfitzgerald cwfitzgerald requested a review from a team May 11, 2024 03:28
teoxoy
teoxoy approved these changes May 14, 2024
View reviewed changes
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.

Looks good!

@teoxoy
Copy link
Member

teoxoy commented May 14, 2024

Could you squash the last 3 commits so that we end up with the refactor + the implementation? We can then land this without squashing it.

@VlaDexa
Copy link
Contributor Author

VlaDexa commented May 14, 2024

Done

@teoxoy teoxoy enabled auto-merge (rebase) May 14, 2024 15:38
@teoxoy teoxoy merged commit 00456cf into gfx-rs:trunk May 14, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teoxoy teoxoy teoxoy approved these changes

@cwfitzgerald cwfitzgerald cwfitzgerald approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement new builtin packing/unpacking functions from WGSL spec
3 participants
@VlaDexa @teoxoy @cwfitzgerald