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

spv-in upgrading AtomicIIncrement type #5775

Merged
merged 2 commits into from
Jun 22, 2024

Conversation

schell
Copy link
Contributor

@schell schell commented Jun 5, 2024

Connections
Partially addresses #4489.
Follow up to #5702.

This creates a new module naga/src/front/atomic_upgrade.rs that begins the upgrade process by recursing through types and expressions previously marked during parsing.

This results in a Module that contains all needed types, but still does not validate, as the upgrades to expressions need to be applied to any expressions that use those expressions until all are resolved.
This results in a Module that validates, at least for the test case.

@jimblandy @cwfitzgerald :)

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.

@schell schell force-pushed the feature/spirv-front-atomics-2 branch from 6dce12d to ca9fa56 Compare June 13, 2024 03:09
@schell schell force-pushed the feature/spirv-front-atomics-2 branch from f6c97c7 to c2a7034 Compare June 13, 2024 04:35
@schell schell marked this pull request as ready for review June 13, 2024 05:14
@schell schell requested a review from a team as a code owner June 13, 2024 05:14
@schell schell changed the title spv-in upgrading AtomicIIncrement spv-in upgrading AtomicIIncrement type Jun 13, 2024
naga/src/lib.rs Outdated Show resolved Hide resolved
naga/tests/in/spv/atomic_i_increment.spv Outdated Show resolved Hide resolved
naga/src/front/atomic_upgrade.rs Outdated Show resolved Hide resolved
naga/src/front/atomic_upgrade.rs Outdated Show resolved Hide resolved
naga/src/front/atomic_upgrade.rs Outdated Show resolved Hide resolved
naga/src/front/atomic_upgrade.rs Outdated Show resolved Hide resolved
naga/src/front/atomic_upgrade.rs Outdated Show resolved Hide resolved
naga/src/front/atomic_upgrade.rs Outdated Show resolved Hide resolved
naga/src/front/atomic_upgrade.rs Outdated Show resolved Hide resolved
naga/src/front/atomic_upgrade.rs Outdated Show resolved Hide resolved
@jimblandy jimblandy force-pushed the feature/spirv-front-atomics-2 branch from b8982b3 to d790a8f Compare June 14, 2024 21:53
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

There are bits and pieces of this that seem right - I think the type upgrading is almost right, for example - but I think the overall approach needs to be changed.

First of all, would be it feasible to restrict our focus for the time being to global variables with atomic or array-of-atomic types, leaving struct members and function arguments that are pointers to atomic values for later PRs?

In that case, what the SPIR-V front end needs to produce is a set of global variables whose types need to be upgraded. Atomics can only appear in workgroup and storage, so they can only really be held in global variables, we don't need to worry about locals or arguments. So instead of the FastHashMap<..., AtomicOp> structure you're producing now, you probably just need an IndexSet<Handle<GlobalVariable>>. The front end can discover the relevant global by looking into the Expression it just constructed for the atomic instruction's pointer operand.

(If the set will be iterated over, we prefer IndexSet over HashSet, to keep the iteration order deterministic.)

Then, the upgrade_atomics function needs to iterate over all those globals and upgrade their types. I think you shouldn't need to update any Expressions at all, so you shouldn't need to mess with any functions or expressions.

naga/src/front/atomic_upgrade.rs Outdated Show resolved Hide resolved
naga/src/front/atomic_upgrade.rs Outdated Show resolved Hide resolved
@schell
Copy link
Contributor Author

schell commented Jun 15, 2024

Ok @jimblandy I'll try what you suggest with regard to tracking IndexSet<Handle<GlobalVariable>> instead of a map of AtomicOp.

@schell schell requested a review from jimblandy June 15, 2024 04:06
@jimblandy
Copy link
Member

The reason I think you won't need to update any expressions is that Naga IR Load expressions and Store statements both can operate on Atomics, so everything accessing the globals whose types you're whacking, whether Loads, Stores, or Atomics, should still be okay.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

Two drive-by comments:

naga/src/front/spv/mod.rs Outdated Show resolved Hide resolved
naga/src/front/spv/mod.rs Outdated Show resolved Hide resolved
@schell schell requested a review from jimblandy June 16, 2024 18:55
@schell schell force-pushed the feature/spirv-front-atomics-2 branch from 6de0d29 to 438aa63 Compare June 16, 2024 19:33
@schell schell mentioned this pull request Jun 16, 2024
24 tasks
@schell schell force-pushed the feature/spirv-front-atomics-2 branch from 438aa63 to 9ec3a2d Compare June 17, 2024 19:54
@jimblandy jimblandy force-pushed the feature/spirv-front-atomics-2 branch from f3a8298 to e9af0fb Compare June 21, 2024 23:30
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

Okay, I've pushed a few more commits here. Please look them over, and if they seem all right, then we can land this.

Be sure to squash the changes when you land it!

naga/src/front/spv/mod.rs Outdated Show resolved Hide resolved
naga/src/front/spv/mod.rs Outdated Show resolved Hide resolved
@jimblandy
Copy link
Member

Oh, um, I guess I should actually fix my review comments if I'm going to approve this

Copy link
Contributor Author

@schell schell left a comment

Choose a reason for hiding this comment

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

@jimblandy these changes all make sense to me. Thank you!

To support atomic instructions in the SPIR-V front end, observe which
global variables the input accesses using atomic instructions, and
adjust their types from ordinary scalars to atomic values.

See comments in `naga::front::atomic_upgrade`.
@jimblandy jimblandy force-pushed the feature/spirv-front-atomics-2 branch from f8e7a3b to 4e50664 Compare June 22, 2024 04:27
@jimblandy jimblandy enabled auto-merge (rebase) June 22, 2024 04:28
@jimblandy
Copy link
Member

Squashed and rebased.

Fixed up some cargo doc issues that I thought should have been caught by CI.

auto-merge was automatically disabled June 22, 2024 04:30

Head branch was pushed to by a user without write access

@schell schell force-pushed the feature/spirv-front-atomics-2 branch from 4e50664 to 6d1b715 Compare June 22, 2024 04:30
@jimblandy jimblandy force-pushed the feature/spirv-front-atomics-2 branch from 6d1b715 to 4e50664 Compare June 22, 2024 04:39
@jimblandy jimblandy enabled auto-merge (rebase) June 22, 2024 04:48
@jimblandy jimblandy merged commit 6405dcf into gfx-rs:trunk Jun 22, 2024
52 checks passed
@schell schell deleted the feature/spirv-front-atomics-2 branch June 26, 2024 00:14
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