← 输入框|github.com|刷新
Skip to content

Rename compute size limits for clarity - #1991

Merged
kvark merged 2 commits into
gpuweb:mainfrom
kainino0x:workgroup-limit-names
Aug 4, 2021
Merged

Rename compute size limits for clarity#1991
kvark merged 2 commits into
gpuweb:mainfrom
kainino0x:workgroup-limit-names

Conversation

@kainino0x

@kainino0x kainino0x commented Jul 26, 2021 •
edited by pr-preview Bot
Loading

Copy link
Copy Markdown
Contributor

Fixes #1929

Preview | Diff

kainino0x
kainino0x commented Jul 26, 2021
View reviewed changes
Comment thread spec/index.bs Outdated
compute stage {{GPUShaderModule}} entry-point.

<tr><td><dfn>maxComputePerDimensionDispatchSize</dfn>
<tr><td><dfn>maxComputeDispatchDimension</dfn>

@kainino0x kainino0x Jul 26, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We didn't discuss this rename previously. I don't really love the name - "dimension" singular isn't nearly as clear as "dimensions" plural.

I wonder if there's any reason we should have separate x/y/z limits for dispatches like we have for workgroup sizes.

@kvark kvark Jul 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maxDispatchGridSize?

@kainino0x kainino0x Jul 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

cc @Kangz

@Kangz Kangz Jul 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maxDispatchSizePerDimension? That or maxDispatchGridSize seems fine.

@kvark kvark Jul 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems to me that both could be confusing wrt whether they are about the numbers passed to dispatch, or the total number of invocations in each dimension.
Perhaps, it would be more explicit to say maxComputeWorkgroupCountPerDimension?

@kainino0x kainino0x Jul 29, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My only reservation with that name is it no longer says "dispatch", which is the place where this validation takes place. But otherwise I like it; applied.

@Kangz Kangz Jul 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/maxComputeWorkgroupsPerDimension/maxDispatchWorkgroupsPerDimension/g ?

@kainino0x
kainino0x requested a review from kvark July 26, 2021 22:15
@github-actions

github-actions Bot commented Jul 26, 2021

Copy link
Copy Markdown
Contributor

Previews, as seen when this build job started (f296505):
WebGPU | IDL
WGSL
Explainer

@github-actions

github-actions Bot commented Jul 29, 2021

Copy link
Copy Markdown
Contributor

Previews, as seen when this build job started (ca44856):
WebGPU | IDL
WGSL
Explainer

@kvark
kvark merged commit 84615d4 into gpuweb:main Aug 4, 2021
github-actions Bot added a commit that referenced this pull request Aug 4, 2021
@kainino0x @github-actions
Rename compute size limits for clarity (#1991)
80b145d 
SHA: 84615d4
Reason: push, by @kvark

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions Bot added a commit that referenced this pull request Aug 4, 2021
@kainino0x @github-actions
Rename compute size limits for clarity (#1991)
98d5db1 
SHA: 84615d4
Reason: push, by @kvark

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions Bot added a commit that referenced this pull request Aug 4, 2021
@kainino0x @github-actions
Rename compute size limits for clarity (#1991)
ff50c4e 
SHA: 84615d4
Reason: push, by @kvark

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kangz Kangz Kangz left review comments
@kvark kvark kvark 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.

Tweak workgroup limit names

3 participants

@kainino0x @kvark @Kangz