-
Notifications
You must be signed in to change notification settings - Fork 373
Add extension subgroup-size-control
#5578
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
Open
Jiawei-Shao
wants to merge
30
commits into
gpuweb:main
Choose a base branch
from
Jiawei-Shao:add-subgroup-size-control
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
f17c4e8
Add extension `subgroup-size-control`
Jiawei-Shao a6d6e7a
Move `subgroup-size-control` and `subgroup_size` to the last
Jiawei-Shao 5f91fdd
Fix a typo
Jiawei-Shao 7837e6d
Simply some statements
Jiawei-Shao d38045f
Fix build error
Jiawei-Shao 502846a
More fixes
Jiawei-Shao 7aaed3a
More fix
Jiawei-Shao 0243e09
More fix
Jiawei-Shao 85d7d06
More fix
Jiawei-Shao 39d36da
Address reviewer's comments
Jiawei-Shao 0d7ecc5
Merge branch 'main' into add-subgroup-size-control
Jiawei-Shao a36b76f
Update correspondence reference
Jiawei-Shao a78a7e3
Fix build error
Jiawei-Shao a02d357
More fix
Jiawei-Shao 07a258e
Merge branch 'main' into add-subgroup-size-control
Jiawei-Shao 22e75bf
Merge branch 'main' into add-subgroup-size-control
Jiawei-Shao 61f89aa
Address reviewer's comments and remove all the added attributes
Jiawei-Shao c870637
Update `correspondence/index.bs`
Jiawei-Shao 07934d0
Address reviewer's comments
Jiawei-Shao ed4d294
Enable `subgroups` automatically with `subgroup-size-control`
Jiawei-Shao 3192a9f
Merge branch 'main' into add-subgroup-size-control
Jiawei-Shao a963977
Address reviewer's comments
Jiawei-Shao 6d584e2
Merge branch 'main' into add-subgroup-size-control
Jiawei-Shao 994434e
Merge branch 'main' into add-subgroup-size-control
Jiawei-Shao ac04c31
Require `subgroups` when enabling `subgroup_size_control` in WGSL
Jiawei-Shao b3741fc
Merge branch 'main' into add-subgroup-size-control
Jiawei-Shao 34971e2
Address reviewer's comments
Jiawei-Shao 09f8c12
Fix typo
Jiawei-Shao 7ae8740
Fix build error
Jiawei-Shao f7741c6
Merge branch 'main' into add-subgroup-size-control
Jiawei-Shao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Kangz marked this conversation as resolved.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| <div class='syntax' noexport='true'> | ||
| <dfn for=syntax>subgroup_size_attr</dfn> : | ||
|
|
||
| <span class="choice"></span> <a for=syntax_sym lt=attr>`'@'`</a> `'subgroup_size'` <a for=syntax_sym lt=paren_left>`'('`</a> [=syntax/expression=] <a for=syntax_sym lt=comma>`','`</a> ? <a for=syntax_sym lt=paren_right>`')'`</a> | ||
| </div> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuing a conversation from gpuweb/cts#4641 (comment) @Jiawei-Shao @jrprice:
I understand that if we set
subgroupMinSizeto 8 (because of fragment) then it's not going to be valid for compute on this device. I guess what I'm saying is if this AdapterInfo isn't going to provide enough information about the hardware to actually use the API, it seems bad.IF it would otherwise be the case that all values between
subgroupMinSizeandsubgroupMaxSizeare valid for trivial pipelines, I really would like to try to maintain that invariant and test it. The test (now removed) suggested this would be true, by arbitrarily choosingsubgroupMaxSizeas the value to test, for all devices except these Intel ones.The options I see for doing that are:
subgroupMinSizeso it only applies to compute - assuming this is a non-starter since it's used for more than justsubgroup-size-controlsubgroupMinSize/subgroupMaxSizesubgroupMinSizeto 16 even for fragment by using subgroup size control to prevent 8 from actually being used in fragment - no clue if this is possibleIF NOT, it doesn't really matter and we can leave the spec as is. The test (when we re-add it) should be changed to keep trying different subgroup sizes until it find one that works. We can use the
vendor/architectureto choose the order in which we try different subgroup sizes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put this test in another PR 4643.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the argument I made in this comment (discussed more in these meeting minutes) was that the value of this limit alone often isn't enough to use the feature either. Knowing that the minimum possible subgroup size is 8 vs 16 vs ... doesn't tell you enough about the memory hierarchy or other hardware properties to decide how your shader should operate. Middleware will want to know if it's Intel vs NVIDIA vs AMD vs ..., and potentially different generations of these architectures, in order to decide how to tile data through memory and registers (for example). At this point they have likely already decided what subgroup size they want based on the architecture, and they won't be considering using 8 on Intel regardless of what the limit says.
I agree that it'd be nice if every value between the limits was reliable usable for a trivial pipeline. My understanding is that over time that will increasingly be true, as it is just this one generation of Intel GPUs that has this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, real applications have more complex requirements. But the issue with this Intel generation seems very specific, just that there's one value that's valid for fragment that's not valid for compute? That seems like something to try to paper over, not something to expose as a wart.
For CTS, there's two reasons I would like to aspirationally test that all values in the range work for a trivial pipeline: (1) so applications can (at least mostly) not worry about that particular detail, (2) so that we can check that the values the implementation exposes are actually the correct ones for the device. The latter is quite valuable IMO.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
James clarified to me offline that my idea to paper over the difference by using the backend's subgroup size control to prevent fragment shaders from ever selecting 8 (even if it would be allowed) won't work, because subgroup size control is compute-only. So seems like papering over is probably not feasible and the only two reasonable options are internal error (the current proposal) or adding a new limit.
The current option does have me somewhat questioning the value of subgroupMinSize/subgroupMaxSize in the first place if sometimes there will be values in that range that just don't work at all. But I guess that ship has sailed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is still feasible? If subgroupMinSize/subgroupMaxSize is going to have very limited use anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrprice @dneto0 What do you think about Kai's comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a breaking change to the API. I don't know how frequently these properties are used for fragment shaders but I'm not sure that we could change them at this stage.