gpuweb icon indicating copy to clipboard operation
gpuweb copied to clipboard

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

Open kainino0x opened this issue 1 year ago • 3 comments

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: https://github.com/gpuweb/gpuweb/pull/1766#discussion_r642648869

However from https://github.com/gpuweb/cts/issues/1991 and https://github.com/gpuweb/gpuweb/pull/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 avatar May 06 '24 19:05 kainino0x

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

kainino0x avatar May 06 '24 19:05 kainino0x

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 avatar May 07 '24 19:05 alan-baker

My interpretation of the https://github.com/gpuweb/cts/issues/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).

alan-baker avatar May 07 '24 19:05 alan-baker

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.

Kangz avatar May 14 '24 22:05 Kangz

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.

kdashg avatar May 14 '24 23:05 kdashg

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 https://github.com/gpuweb/cts/issues/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 avatar May 15 '24 14:05 dneto0

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.

dneto0 avatar May 15 '24 15:05 dneto0

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

kainino0x avatar May 15 '24 17:05 kainino0x

I think https://github.com/gpuweb/gpuweb/pull/4724 addressed this.

teoxoy avatar Jul 10 '24 14:07 teoxoy