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

Does an override need to be statically used by the entry point to be allowed? #4624

Open
kainino0x opened this issue May 6, 2024 · 8 comments
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 wgsl resolved Resolved - waiting for a change to the WGSL specification wgsl WebGPU Shading Language Issues
Milestone

Comments

@kainino0x
Copy link
Contributor

The spec as written says that an override does not need to be statically used by an entry point, to be allowed to be specified in the pipeline descriptor:

  1. For each key → value in descriptor.constants:
    • 1. key must equal the pipeline-overridable constant identifier string of some pipeline-overridable constant defined in the shader module descriptor.module

This was only briefly discussed but it was an intentional choice: #1766 (comment)

However from gpuweb/cts#1991 and #3600 it seems like WGSL has been assuming that it does need to be statically used. I'm not sure if the two specs technically conflict since WebGPU does not use the "interface of a shader" in the validation rule, but they definitely sound like they conflict.

@kainino0x kainino0x added wgsl WebGPU Shading Language Issues needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet api WebGPU API labels May 6, 2024
@kainino0x kainino0x added this to the Milestone 1 milestone May 6, 2024
@kainino0x
Copy link
Contributor Author

This was identified thanks to an issue filed with Dawn here https://crbug.com/338624452

@alan-baker
Copy link
Contributor

I like the way it is right now. I don't see the harm in being able to set overrides in the module that aren't used by the shader. The next rule correctly only requires overrides to be set if they have no default and are used by the entry point.

@alan-baker
Copy link
Contributor

My interpretation of the gpuweb/cts#1991 is that the title doesn't match the description very well. To me, the description says make sure to cover more indirect uses of overrides (e.g. via array element counts).

@Kangz
Copy link
Contributor

Kangz commented May 14, 2024

GPU Web WG 2024-05-08 Pacific-time
  • KN: (summary)
  • MM: What’s the reason it has to match “something in the module”?
    • KN: Typo protection mainly? Technically I don’t think it has to, if we wanted to change the spec here from what was previously proposed.
    • MM: Sounds legit.
  • KG: Mozilla will look at this offline.

@kdashg kdashg modified the milestones: Milestone 1, Milestone 2 May 14, 2024
@kdashg
Copy link
Contributor

kdashg commented May 14, 2024

WGSL 2024-05-14 Minutes
  • KG: Example an override ‘banana’ is in the shader, not used by the entry points, and question 1 is it valid to set ‘banana’ on the pipeline. Question 2. If the override is statically used in the entry point and does not have a default, it must be set in the pipeline. Kai points out that WGSL spec defines “interface of a shader”, that’s not referenced in the API spec. But the specs do work together as intended. There’s a potential for misunderstanding.
  • JB: Like that we have “typo detection”. Another question is whether there are useful diagnostics for saying you’re setting an override that’s not used. And default it to off.
  • DN: These pipeline diagnostics would have to be requested at pipeline creation time. That’s (probably?) something that’s not a compiler diagnostic, but rather an api-level diagnostic that’s needed.
  • JB: So the diagnostics system that we have only applies to shader compilation for now, but not yet pipeline compilation?
  • DN: That’s my intuition yeah
  • JB: So maybe we would need a feature for this? It sounds like something we might want to use. “Good service would include diagnostics at pipeline creation”.
  • DN: The spec does anticipate this, and says that diagnostic issues at pipeline compilation time causes validation failure.
  • AB: We could add such a feature, we just need text in the override section, since that’s where we discuss/validate overrides.
  • KG: So M3 to add something like this?
  • AB: Nothing before M2 IMO
  • JB: M2 sounds good to me
  • -> M2
  • DN: Also though, where do we want to add this? Module-scope attribute? Elsewhere? TBD, but we’ll need to figure that out.

@dneto0
Copy link
Contributor

dneto0 commented May 15, 2024

Summary: Yes, it's ok for pipeline creation to set the value of an override that is not statically used by the shader.

From this part of the API spec:

For each key → value in descriptor.constants:

  1. key must equal the pipeline-overridable constant identifier string of some pipeline-overridable constant defined in the shader module descriptor.module by the rules defined in WGSL identifier comparison.

In particular, the override declaration must exist in the shader but it does not have to be statically used by the entry point.

Summarizing cases:

  • override var in the shader and used by entry point: valid to set pipeline-creation constant
  • override var in the shader and not used by the entry point: valid to set pipeline-creation constant
  • override var does not exist in the shader: not valid to set pipeline-creation constant

@jimblandy 's request is for a diagnostic , defaulted to 'off', which will trigger when setting an override that exists in the shader but is not used by the entry point.

Related @dj2 filed gpuweb/cts#3747 to ensure CTS tests cases where an existing but unused override is either set or defaults to an invalid (e.g. overflowing) value.

@dneto0
Copy link
Contributor

dneto0 commented May 15, 2024

I've forked the diagnostic feature request into its own issue #4643
This issue should be closed once we've landed the clarification to the original question.

@kainino0x
Copy link
Contributor Author

Since that's forked to its own M2 issue, I think this issue is back to M1 now.

@kainino0x kainino0x added the api resolved Resolved - waiting for a change to the API specification label May 15, 2024
@kainino0x kainino0x modified the milestones: Milestone 2, Milestone 1 May 15, 2024
@kainino0x kainino0x added the wgsl resolved Resolved - waiting for a change to the WGSL specification label May 15, 2024
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 wgsl resolved Resolved - waiting for a change to the WGSL specification wgsl WebGPU Shading Language Issues
Projects
None yet
Development

No branches or pull requests

5 participants