commit 63ff5bd4f9e835fbb08de4fd8c3284ad92a2fc5c
author François Beaufort <beaufort.francois@gmail.com> Wed Jun 28 12:41:37 2023 +0000
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Wed Jun 28 12:41:37 2023 +0000
tree afd9193d2a662c848dd2229ebf244ea77698fce2
parent 610e4e627af6a894cc148cef61b04b2136252953

Improve validation error for bind group layout bindings in vertex shader

This CL improves the validation error message for read-write storage
buffer and write-only storage texture bindings used in a vertex shader.

Bug: dawn:1883

Change-Id: I674f16ace1d558880bbe062e111e1dcaa9c51f63
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/138240
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Fr <beaufort.francois@gmail.com>

src/dawn/native/BindGroupLayout.cpp

diff --git a/src/dawn/native/BindGroupLayout.cpp b/src/dawn/native/BindGroupLayout.cpp
index 9cc339d..4324548 100644
--- a/src/dawn/native/BindGroupLayout.cpp
+++ b/src/dawn/native/BindGroupLayout.cpp
@@ -69,12 +69,9 @@
     DAWN_TRY(ValidateShaderStage(entry.visibility));
 
     int bindingMemberCount = 0;
-    BindingInfoType bindingType;
-    wgpu::ShaderStage allowedStages = kAllStages;
 
     if (entry.buffer.type != wgpu::BufferBindingType::Undefined) {
         bindingMemberCount++;
-        bindingType = BindingInfoType::Buffer;
         const BufferBindingLayout& buffer = entry.buffer;
 
         // The kInternalStorageBufferBinding is used internally and not a value
@@ -87,19 +84,21 @@
 
         if (buffer.type == wgpu::BufferBindingType::Storage ||
             buffer.type == kInternalStorageBufferBinding) {
-            allowedStages &= ~wgpu::ShaderStage::Vertex;
+            DAWN_INVALID_IF(
+                entry.visibility & wgpu::ShaderStage::Vertex,
+                "Read-write storage buffer binding is used with a visibility (%s) that contains %s "
+                "(note that read-only storage buffer bindings are allowed).",
+                entry.visibility, wgpu::ShaderStage::Vertex);
         }
     }
 
     if (entry.sampler.type != wgpu::SamplerBindingType::Undefined) {
         bindingMemberCount++;
-        bindingType = BindingInfoType::Sampler;
         DAWN_TRY(ValidateSamplerBindingType(entry.sampler.type));
     }
 
     if (entry.texture.sampleType != wgpu::TextureSampleType::Undefined) {
         bindingMemberCount++;
-        bindingType = BindingInfoType::Texture;
         const TextureBindingLayout& texture = entry.texture;
         // The kInternalResolveAttachmentSampleType is used internally and not a value
         // in wgpu::TextureSampleType.
@@ -133,7 +132,6 @@
 
     if (entry.storageTexture.access != wgpu::StorageTextureAccess::Undefined) {
         bindingMemberCount++;
-        bindingType = BindingInfoType::StorageTexture;
         const StorageTextureBindingLayout& storageTexture = entry.storageTexture;
         DAWN_TRY(ValidateStorageTextureAccess(storageTexture.access));
         DAWN_TRY(ValidateStorageTextureFormat(device, storageTexture.format));
@@ -145,7 +143,10 @@
         }
 
         if (storageTexture.access == wgpu::StorageTextureAccess::WriteOnly) {
-            allowedStages &= ~wgpu::ShaderStage::Vertex;
+            DAWN_INVALID_IF(entry.visibility & wgpu::ShaderStage::Vertex,
+                            "Write-only storage texture binding is used with a visibility (%s) "
+                            "that contains %s.",
+                            entry.visibility, wgpu::ShaderStage::Vertex);
         }
     }
 
@@ -153,7 +154,6 @@
     FindInChain(entry.nextInChain, &externalTextureBindingLayout);
     if (externalTextureBindingLayout != nullptr) {
         bindingMemberCount++;
-        bindingType = BindingInfoType::ExternalTexture;
     }
 
     DAWN_INVALID_IF(bindingMemberCount == 0,
@@ -164,10 +164,6 @@
                     "BindGroupLayoutEntry had more than one of buffer, sampler, texture, "
                     "storageTexture, or externalTexture set");
 
-    DAWN_INVALID_IF(!IsSubset(entry.visibility, allowedStages),
-                    "%s bindings cannot be used with a visibility of %s. Only %s are allowed.",
-                    bindingType, entry.visibility, allowedStages);
-
     return {};
 }
 

src/dawn/native/BindGroupLayout.h

diff --git a/src/dawn/native/BindGroupLayout.h b/src/dawn/native/BindGroupLayout.h
index cfcbf43..c2ffb97 100644
--- a/src/dawn/native/BindGroupLayout.h
+++ b/src/dawn/native/BindGroupLayout.h
@@ -21,7 +21,6 @@
 #include <string>
 
 #include "dawn/common/Constants.h"
-#include "dawn/common/Math.h"
 #include "dawn/common/SlabAllocator.h"
 #include "dawn/common/ityp_span.h"
 #include "dawn/common/ityp_vector.h"