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

validation: More detailed on incompatible BGL #4826

Merged
merged 5 commits into from
Dec 17, 2023

Conversation

scoopr
Copy link
Contributor

@scoopr scoopr commented Dec 4, 2023

When encountering a incompatible BindGroupLayout on RenderPipelines,
attempt to report the actual difference causing the incompatibility.

Description
When encountering a validation error of incompatible layouts on a renderpipeline, the validation error wasn't too helpful.

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(0, 1, Metal)>`
    In a draw command, indexed:true indirect:false
      note: render pipeline = `shadow`
    The pipeline layout, associated with the current render pipeline, contains a bind group layout at index 1 which is incompatible with the bind group layout associated with the bind group at 1

I shortened the final error message a little, as it was confusingly wordy, and then added info on the differences:

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(0, 1, Metal)>`
    In a draw command, indexed:true indirect:false
      note: render pipeline = `shadow`
    The pipeline layout, associated with the current render pipeline, contains a incompatible bind group layout at index 1
      note: Entry 1 not found in assigned bindgroup layout

At this stage, I'm still asking opinions on how the overall approach, and implementation.

Still things to do:

  • Go through the wording a bit more
  • Show the labels of the BGLs
  • Implement the same for compute pipelines
  • .. and the checklist stuff

Testing
Manually munging the examples.

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Some small things, then CI, then good

wgpu-core/src/command/bind.rs Outdated Show resolved Hide resolved
wgpu-core/src/command/bind.rs Outdated Show resolved Hide resolved
When encountering a incompatible BindGroupLayout on RenderPipelines,
attempt to report the actual difference causing the incompatibility.
@scoopr
Copy link
Contributor Author

scoopr commented Dec 17, 2023

Alright, I addressed the nits, tweaked the error messages, and applied the same for compute side. I think this is now in a bit better shape.

Example of with compute looks like:

Caused by:
    In a ComputePass
      note: encoder = `<CommandBuffer-(0, 1, Metal)>`
    In a dispatch command, indirect:false
      note: compute pipeline = `Compute pipeline`
    Incompatible bind group at index 0 in the current compute pipeline
      note: Should be compatible with bind group layout with label = `compute bgl2`
      note: Assigned bind group layout with label = `compute bgl`
      note: Entry 3 not found in assigned bindgroup layout

Few notes,

  • I couldn't use the same labeling infra that the other errors use as I couldn't get the bgl ids nicely, but probably for the best, as I think the added context in the error messages is useful, though the lines are a bit long, and not sure if equals sign is helpful there.
  • As it happens, this type of error message is presented very rarely, as lot of the other validation checks trigger before getting this far, so depending on the case, it might be nice to ease the validation on another layer to make it trigger here, but then again earlier check might have more useful context.
  • And my main concern is actually that the validation check itself is different code than the validation error generation, which might cause some deviation. Its also pretty mechanical code, and it almost triggered me to go learn proc-macros to generate the diff and a fast-path validation "easily" from the same source. But that seemed a lot of effort for just this one problem :)

@scoopr scoopr marked this pull request as ready for review December 17, 2023 20:39
@scoopr scoopr requested a review from a team as a code owner December 17, 2023 20:39
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Amazing!

@cwfitzgerald
Copy link
Member

I think we should give this a spin, then iterate if there are issues.

@cwfitzgerald cwfitzgerald merged commit 159ac9e into gfx-rs:trunk Dec 17, 2023
27 checks passed
@scoopr scoopr deleted the bgl-diff branch December 18, 2023 08:45
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 3, 2024
…225e1744fd7. r=webgpu-reviewers,supply-chain-reviewers,teoxoy

Changelog:

 * #4865 fix present mode for wgl
   By xiaopengli89 in gfx-rs/wgpu#4865
 * #4873 Bump ctor from 0.2.5 to 0.2.6
   By dependabot[bot] in gfx-rs/wgpu#4873
 * #4874 Bump syn from 2.0.40 to 2.0.41
   By dependabot[bot] in gfx-rs/wgpu#4874
 * #4869 [naga wgsl-out] Include the `f` suffix on `f32` literals.
   By jimblandy in gfx-rs/wgpu#4869
 * #4850 [naga wgsl-in] Support abstract operands to binary operators.
   By jimblandy in gfx-rs/wgpu#4850
 * #4850 [naga wgsl-in] Support abstract operands to binary operators.
   By jimblandy in gfx-rs/wgpu#4850
 * #4850 [naga wgsl-in] Support abstract operands to binary operators.
   By jimblandy in gfx-rs/wgpu#4850
 * #4850 [naga wgsl-in] Support abstract operands to binary operators.
   By jimblandy in gfx-rs/wgpu#4850
 * #4850 [naga wgsl-in] Support abstract operands to binary operators.
   By jimblandy in gfx-rs/wgpu#4850
 * #4870 [naga wgsl] Let unary operators accept and produce abstract types.
   By jimblandy in gfx-rs/wgpu#4870
 * #4867 Reintroduce buffer snatching Part 1
   By nical in gfx-rs/wgpu#4867
 * #4882 Bump zerocopy from 0.7.26 to 0.7.31
   By dependabot[bot] in gfx-rs/wgpu#4882
 * #4878 Buffer snatching part 2 - Refactor create_buffer
   By nical in gfx-rs/wgpu#4878
 * #4815 Add `wgpu` crate features for backends
   By daxpedda in gfx-rs/wgpu#4815
 * #4887 Allow clippy::pattern_type_mismatch
   By nical in gfx-rs/wgpu#4887
 * #4886 Document wgpu & wgpu-core features
   By Wumpf in gfx-rs/wgpu#4886
 * #4826 validation: More detailed on incompatible BGL
   By scoopr in gfx-rs/wgpu#4826
 * #4888 Web: add support for more `RawWindowHandle` variants
   By daxpedda in gfx-rs/wgpu#4888
 * #4890 Bump thiserror from 1.0.50 to 1.0.51
   By dependabot[bot] in gfx-rs/wgpu#4890
 * #4880 Simplify `ResourceMaps`
   By nical in gfx-rs/wgpu#4880
 * #4891 Make the naga version in trunk as high as the latest published one
   By nical in gfx-rs/wgpu#4891
 * #4893 Avoid allocating memory every time we might log a label
   By nical in gfx-rs/wgpu#4893
 * #4894 Remove some locks in BindGroup
   By nical in gfx-rs/wgpu#4894
 * #4862 Ensure that DeviceLostCallbackC is always called exactly once
   By bradwerth in gfx-rs/wgpu#4862
 * #4900 Support Device fence sharing with dx12 on Windows
   By sotaroikeda in gfx-rs/wgpu#4900
 * #4903 Bump tokio from 1.35.0 to 1.35.1
   By dependabot[bot] in gfx-rs/wgpu#4903
 * #4895 Check that raw buffers and raw bind groups are valid
   By nical in gfx-rs/wgpu#4895
 * #4901 fix: docs
   By miaobuao in gfx-rs/wgpu#4901
 * #4892 Simplify some code around buffer unmapping
   By nical in gfx-rs/wgpu#4892
 * #4896 Buffer snatching
   By nical in gfx-rs/wgpu#4896
 * #4851 Eagerly release GPU resources when we lose the device.
   By bradwerth in gfx-rs/wgpu#4851
 * #4906 Use nightly for docs
   By cwfitzgerald in gfx-rs/wgpu#4906
 * #4908 Bump syn from 2.0.41 to 2.0.42
   By dependabot[bot] in gfx-rs/wgpu#4908
 * #4909 Bump profiling from 1.0.12 to 1.0.13
   By dependabot[bot] in gfx-rs/wgpu#4909
 * #4910 Bump anyhow from 1.0.75 to 1.0.76
   By dependabot[bot] in gfx-rs/wgpu#4910
 * #4913 Remove id32 Feature
   By cwfitzgerald in gfx-rs/wgpu#4913
 * #4914 Add BGL Deduplication Index Test
   By cwfitzgerald in gfx-rs/wgpu#4914
 * #4921 Fix typo "layout pipeline layout" -> "pipeline layout"
   By HactarCE in gfx-rs/wgpu#4921
 * #4922 Bump winit from 0.29.4 to 0.29.5
   By dependabot[bot] in gfx-rs/wgpu#4922
 * #4924 Inline `document-features` usage, remove dep.
   By ErichDonGubler in gfx-rs/wgpu#4924
 * #4871 Speed up Naga's `cargo xtask validate wgsl` from 12s to 0.8s
   By jimblandy in gfx-rs/wgpu#4871
 * #4871 Speed up Naga's `cargo xtask validate wgsl` from 12s to 0.8s
   By jimblandy in gfx-rs/wgpu#4871
 * #4871 Speed up Naga's `cargo xtask validate wgsl` from 12s to 0.8s
   By jimblandy in gfx-rs/wgpu#4871
 * #4930 Bump winit from 0.29.5 to 0.29.6
   By dependabot[bot] in gfx-rs/wgpu#4930
 * #4929 Bump web-time from 0.2.3 to 0.2.4
   By dependabot[bot] in gfx-rs/wgpu#4929
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4940 Align `wgpu_types::CompositeAlphaMode` serde serializations to spec
   By littledivy in gfx-rs/wgpu#4940
 * #4936 Bump anyhow from 1.0.76 to 1.0.77
   By dependabot[bot] in gfx-rs/wgpu#4936
 * #4933 Bump thiserror from 1.0.51 to 1.0.52
   By dependabot[bot] in gfx-rs/wgpu#4933
 * #4932 Bump syn from 2.0.42 to 2.0.43
   By dependabot[bot] in gfx-rs/wgpu#4932

Differential Revision: https://phabricator.services.mozilla.com/D197519
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Jan 5, 2024
…225e1744fd7. r=webgpu-reviewers,supply-chain-reviewers,teoxoy

Changelog:

 * #4865 fix present mode for wgl
   By xiaopengli89 in gfx-rs/wgpu#4865
 * #4873 Bump ctor from 0.2.5 to 0.2.6
   By dependabot[bot] in gfx-rs/wgpu#4873
 * #4874 Bump syn from 2.0.40 to 2.0.41
   By dependabot[bot] in gfx-rs/wgpu#4874
 * #4869 [naga wgsl-out] Include the `f` suffix on `f32` literals.
   By jimblandy in gfx-rs/wgpu#4869
 * #4850 [naga wgsl-in] Support abstract operands to binary operators.
   By jimblandy in gfx-rs/wgpu#4850
 * #4850 [naga wgsl-in] Support abstract operands to binary operators.
   By jimblandy in gfx-rs/wgpu#4850
 * #4850 [naga wgsl-in] Support abstract operands to binary operators.
   By jimblandy in gfx-rs/wgpu#4850
 * #4850 [naga wgsl-in] Support abstract operands to binary operators.
   By jimblandy in gfx-rs/wgpu#4850
 * #4850 [naga wgsl-in] Support abstract operands to binary operators.
   By jimblandy in gfx-rs/wgpu#4850
 * #4870 [naga wgsl] Let unary operators accept and produce abstract types.
   By jimblandy in gfx-rs/wgpu#4870
 * #4867 Reintroduce buffer snatching Part 1
   By nical in gfx-rs/wgpu#4867
 * #4882 Bump zerocopy from 0.7.26 to 0.7.31
   By dependabot[bot] in gfx-rs/wgpu#4882
 * #4878 Buffer snatching part 2 - Refactor create_buffer
   By nical in gfx-rs/wgpu#4878
 * #4815 Add `wgpu` crate features for backends
   By daxpedda in gfx-rs/wgpu#4815
 * #4887 Allow clippy::pattern_type_mismatch
   By nical in gfx-rs/wgpu#4887
 * #4886 Document wgpu & wgpu-core features
   By Wumpf in gfx-rs/wgpu#4886
 * #4826 validation: More detailed on incompatible BGL
   By scoopr in gfx-rs/wgpu#4826
 * #4888 Web: add support for more `RawWindowHandle` variants
   By daxpedda in gfx-rs/wgpu#4888
 * #4890 Bump thiserror from 1.0.50 to 1.0.51
   By dependabot[bot] in gfx-rs/wgpu#4890
 * #4880 Simplify `ResourceMaps`
   By nical in gfx-rs/wgpu#4880
 * #4891 Make the naga version in trunk as high as the latest published one
   By nical in gfx-rs/wgpu#4891
 * #4893 Avoid allocating memory every time we might log a label
   By nical in gfx-rs/wgpu#4893
 * #4894 Remove some locks in BindGroup
   By nical in gfx-rs/wgpu#4894
 * #4862 Ensure that DeviceLostCallbackC is always called exactly once
   By bradwerth in gfx-rs/wgpu#4862
 * #4900 Support Device fence sharing with dx12 on Windows
   By sotaroikeda in gfx-rs/wgpu#4900
 * #4903 Bump tokio from 1.35.0 to 1.35.1
   By dependabot[bot] in gfx-rs/wgpu#4903
 * #4895 Check that raw buffers and raw bind groups are valid
   By nical in gfx-rs/wgpu#4895
 * #4901 fix: docs
   By miaobuao in gfx-rs/wgpu#4901
 * #4892 Simplify some code around buffer unmapping
   By nical in gfx-rs/wgpu#4892
 * #4896 Buffer snatching
   By nical in gfx-rs/wgpu#4896
 * #4851 Eagerly release GPU resources when we lose the device.
   By bradwerth in gfx-rs/wgpu#4851
 * #4906 Use nightly for docs
   By cwfitzgerald in gfx-rs/wgpu#4906
 * #4908 Bump syn from 2.0.41 to 2.0.42
   By dependabot[bot] in gfx-rs/wgpu#4908
 * #4909 Bump profiling from 1.0.12 to 1.0.13
   By dependabot[bot] in gfx-rs/wgpu#4909
 * #4910 Bump anyhow from 1.0.75 to 1.0.76
   By dependabot[bot] in gfx-rs/wgpu#4910
 * #4913 Remove id32 Feature
   By cwfitzgerald in gfx-rs/wgpu#4913
 * #4914 Add BGL Deduplication Index Test
   By cwfitzgerald in gfx-rs/wgpu#4914
 * #4921 Fix typo "layout pipeline layout" -> "pipeline layout"
   By HactarCE in gfx-rs/wgpu#4921
 * #4922 Bump winit from 0.29.4 to 0.29.5
   By dependabot[bot] in gfx-rs/wgpu#4922
 * #4924 Inline `document-features` usage, remove dep.
   By ErichDonGubler in gfx-rs/wgpu#4924
 * #4871 Speed up Naga's `cargo xtask validate wgsl` from 12s to 0.8s
   By jimblandy in gfx-rs/wgpu#4871
 * #4871 Speed up Naga's `cargo xtask validate wgsl` from 12s to 0.8s
   By jimblandy in gfx-rs/wgpu#4871
 * #4871 Speed up Naga's `cargo xtask validate wgsl` from 12s to 0.8s
   By jimblandy in gfx-rs/wgpu#4871
 * #4930 Bump winit from 0.29.5 to 0.29.6
   By dependabot[bot] in gfx-rs/wgpu#4930
 * #4929 Bump web-time from 0.2.3 to 0.2.4
   By dependabot[bot] in gfx-rs/wgpu#4929
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4902 [naga xtask] Run validation jobs in parallel, using jobserver.
   By jimblandy in gfx-rs/wgpu#4902
 * #4940 Align `wgpu_types::CompositeAlphaMode` serde serializations to spec
   By littledivy in gfx-rs/wgpu#4940
 * #4936 Bump anyhow from 1.0.76 to 1.0.77
   By dependabot[bot] in gfx-rs/wgpu#4936
 * #4933 Bump thiserror from 1.0.51 to 1.0.52
   By dependabot[bot] in gfx-rs/wgpu#4933
 * #4932 Bump syn from 2.0.42 to 2.0.43
   By dependabot[bot] in gfx-rs/wgpu#4932

Differential Revision: https://phabricator.services.mozilla.com/D197519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants