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

Subgroup Operations #5301

Merged
merged 89 commits into from
Apr 17, 2024
Merged

Subgroup Operations #5301

merged 89 commits into from
Apr 17, 2024

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Feb 25, 2024 •
edited by teoxoy
Loading

Additional PR for code review as requested in #4190 (comment).

Resolves #4428

exrook and others added 30 commits October 25, 2023 13:55
@exrook
subgroup: Implement subgroupBallot for wgsl-in, wgsl-out, spv-out, hl…
14f6be2 
…sl-out

TODO: metal out, figure out what needs to be done in validation
@exrook
subgroup: subgroupBallot metal out
e969f09
@exrook
subgroup: require GroupNonUnifomBallot capability
05feb88
@exrook
subgroup: Add subgroup invocation id and subgroup size builtins
2ff8809
@exrook
subgroup: SubgroupInvocationId is only valid in compute stages
8cbf423
@exrook
subgroup: expierment with subgroupBarrier() based on OpControlbarrier
9430603 
SPIR-V OpControlBarrier with execution scope Subgroup has implementation
defined behavior when executed nonuniformly. OpenCL SPIR-V execution
spec say nonuniform execution is UB. Vulkan SPIR-V execution spec says
nothing :).
@exrook
subgroup: add statement for rest of subgroup ops
44f6929
@exrook
subgroup: fix doc error on SubgroupBroadcast
b5be66e
@exrook
subgroup: wgsl-in and spv-out for subgroup operations
a277ec5
@exrook
subgroup: add optional predicate for subgroupBallot
dd49f99
@Lichtso @exrook
Renames SubgroupBroadcast => SubgroupGather and BroadcastMode => Gath…
18ceb01 
…erMode.
@Lichtso @exrook
General fixes.
7c911ba
@Lichtso @exrook
Adds BuiltIn::NumSubgroups, BuiltIn::SubgroupId.
f5c4ad7
@Lichtso @exrook
Adds GatherMode::Shuffle, GatherMode::ShuffleDown, GatherMode::Shuffl…
f931a47 
…eUp, GatherMode::ShuffleXor.
@Lichtso @exrook
Implements all frontends and backends.
d8e17e7
@Lichtso @exrook
Adjusts metal backend test_stack_size().
43133d0
@Lichtso @exrook
Adds test and snapshots.
275a5a5
@exrook
subgroup: fix spv-in
f04b71d
@exrook
subgroup: Add 0 arg subgroupBallot to tests and fix wgsl-out whitespace
7a5bf0f
@exrook
subgroup: Add spv-in test
2aaf0a0
@exrook
subgroup: resolve fixmes & fix typo
fb1e3f9
@exrook
subgroup: Add subgroup capability
209225a
@exrook
subgroup: Treat subgroup operation results as non-uniform
77d33b9
@exrook
subgroup: refactor wgsl subgroup gather parsing
1e51650
@exrook
subgroup: doc comments for subgroup Statements and Expressions
68fc4d5
@exrook
subgroup: add validation for each subgroup operation type
91c569d 
supported operations and stages subgroup operations are supported on
can be passed to the validator after creating it

operations are grouped to follow vulkan:
- basic: elect, barrier
- vote: any, all
- arithmetic: reductions, scan
- ballot: ballot, broadcasts,
- shuffle: shuffles,
- shuffle relative: shuffle up, down
@exrook
Add feature for subgroup operations in fragment and compute shaders
6f6f789
@exrook
Adds feature detection for Vulkan.
7d2e273
@Lichtso @exrook
Adds feature detection for Metal.
55e21e8
@Lichtso @exrook
Adds feature detection for DirectX 12.
2e44fb1
@Lichtso
Copy link
Contributor Author

Lichtso commented Apr 9, 2024

I fiddled with the Vulkan backend since yesterday and it is still complaining that:

Validation Error: Validation Error: [ VUID-VkPipelineShaderStageCreateInfo-flags-02784 ] | MessageID = 0xd65f4ec3 | vkCreateGraphicsPipelines(): pCreateInfos[0].pStages[0].flags includes VK_PIPELINE_SHADER_STAGE_CREATE_ALLOW_VARYING_SUBGROUP_SIZE_BIT_EXT, but the subgroupSizeControl feature was not enabled. The Vulkan spec states: If flags has the VK_PIPELINE_SHADER_STAGE_CREATE_ALLOW_VARYING_SUBGROUP_SIZE_BIT flag set, the subgroupSizeControl feature must be enabled (https://vulkan.lunarg.com/doc/view/1.3.268.0/linux/1.3-extensions/vkspec.html#VUID-VkPipelineShaderStageCreateInfo-flags-02784)

Even though I coupled the setting of PipelineShaderStageCreateFlags directly to the presence of the feature flag:

subgroup_size_control: phd_features
     .subgroup_size_control
     .map_or(false, |ext| ext.subgroup_size_control == vk::TRUE),

if self.shared.private_caps.subgroup_size_control {
     flags |= vk::PipelineShaderStageCreateFlags::ALLOW_VARYING_SUBGROUP_SIZE;
}

@teoxoy Ideas?

@teoxoy
Copy link
Member

teoxoy commented Apr 9, 2024

Ah yes, we need to explicitly enable it when creating the device.

I think the only thing missing is actually pushing the struct.

wgpu/wgpu-hal/src/vulkan/adapter.rs

Line 150 in 82dead0

}

@teoxoy
Copy link
Member

teoxoy commented Apr 9, 2024

I think the last remaining thing is #5301 (comment).

teoxoy
teoxoy reviewed Apr 10, 2024
View reviewed changes
naga/src/valid/interface.rs Outdated Show resolved Hide resolved
@atlv24
Copy link
Contributor

atlv24 commented Apr 15, 2024

Merge conflicts and compile errors fixed here: Lichtso#1

teoxoy
teoxoy approved these changes 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.

I think it's time 🎉

Thank you for this substantial contribution!

@teoxoy
Copy link
Member

teoxoy commented Apr 15, 2024

@cwfitzgerald do you want to give this another look? Since you've previously requested changes.

@FL33TW00D FL33TW00D mentioned this pull request Apr 16, 2024
Closed
@JMS55 JMS55 mentioned this pull request Apr 17, 2024
Merged
cwfitzgerald
cwfitzgerald approved these changes Apr 17, 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.

Amazing work everyone!

@cwfitzgerald cwfitzgerald merged commit ea77d56 into gfx-rs:trunk Apr 17, 2024
25 checks passed
@Lichtso Lichtso deleted the subgroup_feature branch April 17, 2024 19:59
@Lichtso
Copy link
Contributor Author

Lichtso commented Apr 17, 2024

@exrook We did it! The subgroup feature is merged.

@teoxoy You might want to adjust the CHANGELOG, I think we accidentally categorized it as "Bug fix" not "New features".

@cwfitzgerald
Copy link
Member

I can touch up the changelog, there is a merge conlfict on trunk between this and #5508 that I'm fixing

@cwfitzgerald cwfitzgerald mentioned this pull request Apr 17, 2024
Merged
cwfitzgerald added a commit that referenced this pull request Apr 17, 2024
@cwfitzgerald
Fix Merge Issues Between #5301 and #5508 (#5549)
c1291bd
@teoxoy teoxoy mentioned this pull request Apr 18, 2024
Open
@teoxoy
Copy link
Member

teoxoy commented Apr 18, 2024

I put together a new issue to track the differences between our implementation and the WebGPU proposal #5555.

@FL33TW00D FL33TW00D mentioned this pull request Jun 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raphlinus raphlinus raphlinus left review comments

@atlv24 atlv24 atlv24 left review comments

@JMS55 JMS55 JMS55 left review comments

@cwfitzgerald cwfitzgerald cwfitzgerald approved these changes

@teoxoy teoxoy teoxoy 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.

Support subgroup/wave operations
8 participants
@Lichtso @vgarleanu @teoxoy @raphlinus @atlv24 @cwfitzgerald @JMS55 @exrook