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

Do pipeline-creation errors encompass the whole module or just the GPUProgrammableStage? #4648

Open
alan-baker opened this issue May 16, 2024 · 5 comments
Labels
api WebGPU API wgsl WebGPU Shading Language Issues
Milestone

Comments

@alan-baker
Copy link
Contributor

Related to #4624 and #4643.

Neither spec is clear on the content that is checked for pipeline-creation errors. Does it just encompass the code needed for the GPUProgrammableStage or the whole module? #4643 describes a diagnostic on something outside the code required for a GPUProgrammableStage so that might imply errors are from the whole module, but I don't remember either group ever making a conscious decision about this.

Example 1: module scope

override unused : i32;
override unused2 = clamp(0, unused, 1);
@compute @workgroup_size(1)
fn main() { }

When compiling main if we set unused to a value > 1 should this produce an error?

Example 2: used in different shader

override x = 10i;
@compute @workgroup_size(1)
fn foo() {
  let tmp = clamp(0, x, 1);
}
@compute @workgroup_size(1)
fn bar() { }

When compiling bar is an error produced if the value of x is not set?

For reference (but not to influence any decision), Dawn does not error in either case.

cc: @dneto0 @dj2 @kainino0x @jimblandy

@mighdoll
Copy link

From the perspective of writing an external linker, I think it’s probably a little better to ignore errors in code unused by the stage. I don’t think it would matter much in my current implementation of wgsl-linker. But earlier versions left some unreferenced code in the linked result, and maybe some other tools will do that.

So weak vote for being permissive about errors in unused code for the sake of future external wgsl code manglers.

@greggman
Copy link
Contributor

I'm not sure if this is the same issue but does this produce an error if I only reference bar? It feels like it does given the recent decision about validating things at shader module creation over pipeline creation (#4568)

@compute @workgroup_size(10000000000)
fn foo() {
}
@compute @workgroup_size(1)
fn bar() { }

So it feels like the example with overrides would generate an error given the unused paths covered by #4568 also generate an error.

@alan-baker
Copy link
Contributor Author

I'm not sure if this is the same issue but does this produce an error if I only reference bar? It feels like it does given the recent decision about validating things at shader module creation over pipeline creation (#4568)

@compute @workgroup_size(10000000000)
fn foo() {
}
@compute @workgroup_size(1)
fn bar() { }

So it feels like the example with overrides would generate an error given the unused paths covered by #4568 also generate an error.

Currently the only shader-creation error for workgroup_size is for a non-positive value. The upper limit is phrased as a pipeline-creation error. So it would be a similar question.

@alan-baker
Copy link
Contributor Author

We discussed this internally today. We think that pipeline-creation should be restricted to the stage being compiled generally. There are a few interesting cases that ought to lead to spec clarifications though.

Consider the following:

override a : i32 = 0;
override b = 1 / a;
override c = a / 0;

c should always result in a shader-creation error because of the integer division rules.

For b there are a few cases:

  • b is statically used by the GPUProgrammableStage shader:
    • If b is overridden, no error
    • If a is overridden to non-zero, no error
    • If a is 0 and b is not overridden, pipeline-creation error
  • b is not statically used by the GPUProgrammableStage shader, no error

It is possible the spec requires more clarification in override expressions to clarify the substitution of values happens before any expression evaluation and that it might cause some expressions to not be evaluated.

@kdashg
Copy link
Contributor

kdashg commented Jun 4, 2024

WGSL 2024-06-04 Minutes
  • AB: This was prompted by other discussion: setting overrides unused by the stage. Realized we should be clear what is compiled as part of the shader stage. Our implementation and intuition is it’s only what’s needed to compile the stage. Thing outside that are not considered; has knock-on effects (e.g. diagnostic in WGSL for set-but-unused-overrides if they’re stripped away). Discussion prompted examples in the issue. Do pipeline-creation errors encompass the whole module or just the GPUProgrammableStage? #4648 (comment)
    •   Consider the following:
      
        override a : i32 = 0;
        override b = 1 / a;
        override c = a / 0;
      
      
      
        c should always result in a shader-creation error because of the integer division rules.
        For b there are a few cases:
        b is statically used by the GPUProgrammableStage shader:
        If b is overridden, no error
        If a is overridden to non-zero, no error
        If a is 0 and b is not overridden, pipeline-creation error
        b is not statically used by the GPUProgrammableStage shader, no error
        It is possible the spec requires more clarification in override expressions to clarify the substitution of values happens before any expression evaluation and that it might cause some expressions to not be evaluated.
      
    • Also have to clarify how and when constants-value substitution occurs, and when validation occurs w.r.t. To that.
    • We realize there is some missing CTS as well.
  • JB: I partly proposed #... we were told it’s very common to set a whole packet of overrides whether or not the value is used in the stage. So the diagnostic may have marginal use. I haven’t read through 4648. I don’t feel I understand all the issue. If 4643 is the motivating concern.
  • AB: 4648 is an issue on its own . The spec is not clear what is being included.
  • JB: I ask for more time to review.
  • KG: Gamedevs may want the lax behavior. Think there’s another community of users who want the stricter behaviour.
  • JB: I would turn it on.
  • DS: Question for 4648 is how does validation interact with overrides that are not used. If you didn’t use an override but would ordinarily be an error (if it were used), then should it be?
  • AB: Careful about shader-creation vs. pipeline-creation error.

@dneto0 dneto0 added this to the Milestone 1 milestone Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api WebGPU API wgsl WebGPU Shading Language Issues
Projects
None yet
Development

No branches or pull requests

5 participants