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

Remove maxInterStageShaderComponents #4783

Merged
merged 1 commit into from
Aug 28, 2024
Merged

Conversation

kainino0x
Copy link
Contributor

@kainino0x kainino0x commented Jul 25, 2024

Prefer to land this after #4781, if we accept that Done and rebased

Fixes #4688

@kainino0x kainino0x added needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet api resolved Resolved - waiting for a change to the API specification api WebGPU API labels Jul 25, 2024
Copy link
Contributor

github-actions bot commented Jul 25, 2024

Previews, as seen when this build job started (1696617):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

@kainino0x
Copy link
Contributor Author

kainino0x commented Jul 25, 2024

For the record here"s how I"m thinking we can deprecate/remove this from Chromium (taking slight advantage of #4781):

    • Update validation to match this PR, leaving maxInterStageShaderComponents unused by validation.
      • This should not be a breaking change because it was already necessary to raise maxInterStageShaderVariables to take advantage of it (there might be some VERY particular situation where you could raise maxInterStageShaderComponents without raising maxInterStageShaderVariables but if so it"s very unlikely to be used).
    • Change GPUSupportedLimits.maxInterStageShaderComponents (adapter.limits.maxInterStageShaderComponents or device.limits.maxInterStageShaderComponents) to return maxInterStageShaderVariables * 4.
      • This could also technically break something because the device won"t expose the same value you requested, but that seems equally unlikely to break anything (except CTS).
      • Now requiredLimits still accepts maxInterStageShaderComponents, but doesn"t do anything with it.
    • Deprecate both the GPUSupportedLimits.maxInterStageShaderComponents accessor and requiredLimits.maxInterStageShaderComponents.
  1. Watch deprecation telemetry to make sure usage goes down.
  2. Remove the limit from GPUSupportedLimits and stop accepting it in requiredLimits.
    • Note that Allow unknown limits to be requested with value undefined #4781 means any sites doing { requiredLimits: { maxInterStageShaderComponents: adapter.limits.maxInterStageShaderComponents } } would actually not break even though they would produce deprecation warnings. We can"t detect this robustly.
    • If we don"t do that, Chromium can keep a quirk to do that, but only for maxInterStageShaderComponents, as someone suggested in last week"s meeting.

@kainino0x kainino0x marked this pull request as ready for review July 25, 2024 01:07
@kainino0x kainino0x requested a review from toji July 25, 2024 01:07
@kainino0x kainino0x added this to the Milestone 0 milestone Jul 25, 2024
@kainino0x kainino0x added the potentially breaking Could require a breaking change to the API label Jul 26, 2024
Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

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

LGTM, though I share Kai"s preference for landing after #4781

@kainino0x
Copy link
Contributor Author

(Rebased with no change after landing #4781)

@kainino0x kainino0x merged commit bda63a8 into gpuweb:main Aug 28, 2024
4 checks passed
@kainino0x kainino0x deleted the components branch August 28, 2024 21:17
juj added a commit to juj/wasm_webgpu that referenced this pull request Sep 6, 2024
copybara-service bot pushed a commit to google/dawn that referenced this pull request Sep 18, 2024
gpuweb/gpuweb@23db3e4...2dc56f2

 * Handle device limits being undefined
   * gpuweb/gpuweb#4781
 * Remove the maxInterStageShaderComponents limit
   * gpuweb/gpuweb#4783

Bug: 368022291
Change-Id: I548a1c2e841aa40feb0234214a9dc75289ffcb13
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/207095
Commit-Queue: Geoff Lang <[email protected]>
Reviewed-by: Corentin Wallez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api resolved Resolved - waiting for a change to the API specification api WebGPU API needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet potentially breaking Could require a breaking change to the API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

maxInterStageShaderComponents seems to overlap with maxInterStageShaderVariables
2 participants