-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Migrate validation from maxInterStageShaderComponents to maxInterStageShaderVariables (minus coverage of clip-distances)
#8652
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
Conversation
maxInterStageShaderComponents to maxInterStageShaderVariables` maxInterStageShaderComponents to maxInterStageShaderVariables
b948e13 to
853e129
Compare
853e129 to
3951f50
Compare
maxInterStageShaderComponents to maxInterStageShaderVariables maxInterStageShaderVariables validation
3951f50 to
32215a3
Compare
maxInterStageShaderVariables validationmaxInterStageShaderVariables validation (minus coverage of clip-distances)
42456c9 to
3354ba5
Compare
maxInterStageShaderVariables validation (minus coverage of clip-distances)maxInterStageShaderComponents to maxInterStageShaderVariables (minus coverage of clip-distances)
6f1e549 to
a9d5a27
Compare
a9d5a27 to
2c18228
Compare
|
Oh, wow. When I'm running CTS for non- I don't plan on changing any end-state code at this point—just test coverage and commit structure—so I think this is ready for review. |
|
@andyleiserson said he'd review this in an internal Mozilla meeting today. ❤️ |
| SubgroupSize, | ||
|
|
||
| // Non-standard | ||
| // TODO: Is this list actually good? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanted to explicitly call out a request for reviewer opinion here. Particularly important to get right is (1) what this set actually is, and (2) how much space each built-in should take up.
Based on the commentary on variants of naga::BuiltIn and some skimming of built-in implementation to (non-exhaustively) confirm things, I believe this is correct.
438f534 to
2ae309c
Compare
|
Hmm, I noticed that running the EDIT: Filed #8777. |
2ae309c to
76bdf80
Compare
andyleiserson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
| max_compute_workgroup_size_z: 0, | ||
| max_compute_workgroups_per_dimension: 0, | ||
|
|
||
| // Value supported by Intel Celeron B830 on Windows (OpenGL 3.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Value supported by Intel Celeron B830 on Windows (OpenGL 3.1) | |
| // Intel Celeron B830 on Windows (OpenGL 3.1) supports up to 31 bytes (~8 variables) |
(Or delete it entirely. It feels like a shame to lose the research but it may not be worth maintaining.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is bytes and words, or words and vec4s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kpreid is the author of this code originally, so let's ping him and ask! Kevin, Do you recall what the actual capabilities are here? If we need to, then this is an interesting conflict with the spec., in which even compatibility mode only lowers the minimum support required to 15. I'm guessing that, as @andyleiserson suspects, we should only allow 8 variables here?
I'll file an issue and follow-up if there are changes we need to make here before releasing. I'm still gonna merge, tho. ![]()
…erStage` This will better prepare for the subsequent commit, which supplants `naga::ShaderStage` as the source of truth in this code.
76bdf80 to
d3eb071
Compare
|
I've added a |
d3eb071 to
8694e44
Compare
…orValidation`
In order to prepare for spec.-compliant validation of shader I/O, we
need to create a new `enum` that will have the same set of _variants_ as
`naga::ShaderStage`, but will carry additional information relevant to
validation. As a preparatory step, create the new `enum` (called
`ShaderStageForValidation`), and use it as the source of truth for
details of the stage being validated. This yields several immediate
benefits, even before making adjustments for the new validation:
- Stage-specific stage is now properly encapsulated. `compare_function`,
which was previously passed as a `Option`al sibling argument, is now
properly associated with only the vertex shader stage.
- This led to an odd discovery that `compare_function` was being
provided to `check_stages` with other stages, but not actually
actually checked. This seems like a rightfully squashed bug.
- Instead of passing a `wgpu_types::ShaderStages` (with a single bit)
and `naga::ShaderStage` side-by-side, encapsulate both into conversion
methods for an instance of `ShaderStageForValidation`, so errors are
impossible to make.
…tion (minus `clip-distances`) Notable public API additions: - New `StageError` variants: - `VertexOutputLocationTooLarge` - `TooManyVertexOutputs` - Add new `shader_io_deductions` submodule of `wgpu_core::validation` with: - A `ShaderIoDeduction` trait - `enum MaxVertexShaderOutputDeduction` - `MaxInterStageShaderVariablesDeductions` for them.
…face::check_stage`
8694e44 to
83b6827
Compare
Recommended review experience is commit-by-commit.
Connections
maxInterStageShaderVariableswas originally discussed for introduction to the WebGPU spec. committee at Should we constrain the location of user input-output stage variables WGSL gpuweb/gpuweb#1962.maxInterStageShaderVariableswas proposed to outright supersedemaxInterStageShaderComponentswith maxInterStageShaderComponents seems to overlap with maxInterStageShaderVariables gpuweb/gpuweb#4688.clip-distancesvalidation is scoped to Add clip distances validation formaxInterStageShaderVariables#8762.Testing
CTS tests have been enabled that previously failed.
Squash or Rebase?
Rebase.