commit 7ae959da60779cef93ebdb118853662b84abe1da
author Corentin Wallez <cwallez@chromium.org> Mon Jul 05 15:07:49 2021 +0000
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Jul 05 15:07:49 2021 +0000
tree d258ac5956090a123a7d76d603f45a738cd9b7e4
parent 3be120026dd329de022cfb99a0664647e82038df

Allowed mismatch sampler/comparison sampler when using SPIRV-Cross

When SPIRV-Cross we always reflect samplers as regular samplers. A
recent change implementing proper validation broke programs using
comparison samplers in that path.

Skip that bit of validation when using SPIRV-Cross, that hack can be
removed once we use Tint unconditionally for reflection.

Also reenables some BGL-ShaderModule compatibility tests with
SPIRV-Cross to cover the "fixed" validation.

Bug: dawn:571
Bug: dawn:367
Change-Id: I2d1a65aaea717b4927ac4e838942547a1b413d33
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/56900
Commit-Queue: Ben Clayton <bclayton@google.com>
Auto-Submit: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Ben Clayton <bclayton@google.com>

src/dawn_native/ShaderModule.cpp

diff --git a/src/dawn_native/ShaderModule.cpp b/src/dawn_native/ShaderModule.cpp
index e201251..014c13e 100644
--- a/src/dawn_native/ShaderModule.cpp
+++ b/src/dawn_native/ShaderModule.cpp
@@ -484,7 +484,7 @@
             return std::move(result);
         }
 
-        MaybeError ValidateCompatibilityWithBindGroupLayout(DeviceBase*,
+        MaybeError ValidateCompatibilityWithBindGroupLayout(DeviceBase* device,
                                                             BindGroupIndex group,
                                                             const EntryPointMetadata& entryPoint,
                                                             const BindGroupLayoutBase* layout) {
@@ -622,6 +622,13 @@
                     }
 
                     case BindingInfoType::Sampler:
+                        // Allow mismatched samplers when using SPIRV-Cross since we can't reflect
+                        // data that's precise enough.
+                        // TODO(dawn:571): Remove once we use Tint unconditionnally for reflection.
+                        if (!device->IsToggleEnabled(Toggle::UseTintGenerator)) {
+                            break;
+                        }
+
                         if ((layoutInfo.sampler.type == wgpu::SamplerBindingType::Comparison) !=
                             shaderInfo.sampler.isComparison) {
                             return DAWN_VALIDATION_ERROR(

src/tests/unittests/validation/BindGroupValidationTests.cpp

diff --git a/src/tests/unittests/validation/BindGroupValidationTests.cpp b/src/tests/unittests/validation/BindGroupValidationTests.cpp
index b6031dd..a3035c8 100644
--- a/src/tests/unittests/validation/BindGroupValidationTests.cpp
+++ b/src/tests/unittests/validation/BindGroupValidationTests.cpp
@@ -2359,9 +2359,6 @@
 // Test that the use of sampler and comparison_sampler in the shader must match the bind group
 // layout.
 TEST_F(SamplerTypeBindingTest, ShaderAndBGLMatches) {
-    // Tint needed for proper shader reflection.
-    DAWN_SKIP_TEST_IF(!HasToggleEnabled("use_tint_generator"));
-
     // Test that a filtering sampler binding works with normal sampler in the shader.
     {
         wgpu::BindGroupLayout bindGroupLayout = utils::MakeBindGroupLayout(
@@ -2399,7 +2396,7 @@
     }
 
     // Test that filtering sampler binding does not work with comparison sampler in the shader.
-    {
+    if (HasToggleEnabled("use_tint_generator")) {
         wgpu::BindGroupLayout bindGroupLayout = utils::MakeBindGroupLayout(
             device, {{0, wgpu::ShaderStage::Fragment, wgpu::SamplerBindingType::Filtering}});
 
@@ -2411,7 +2408,7 @@
     }
 
     // Test that non-filtering sampler binding does not work with comparison sampler in the shader.
-    {
+    if (HasToggleEnabled("use_tint_generator")) {
         wgpu::BindGroupLayout bindGroupLayout = utils::MakeBindGroupLayout(
             device, {{0, wgpu::ShaderStage::Fragment, wgpu::SamplerBindingType::NonFiltering}});
 
@@ -2423,7 +2420,7 @@
     }
 
     // Test that comparison sampler binding does not work with normal sampler in the shader.
-    {
+    if (HasToggleEnabled("use_tint_generator")) {
         wgpu::BindGroupLayout bindGroupLayout = utils::MakeBindGroupLayout(
             device, {{0, wgpu::ShaderStage::Fragment, wgpu::SamplerBindingType::Comparison}});
 
@@ -2505,7 +2502,7 @@
     }
 
     // Test that a filtering sampler cannot be used to sample an unfilterable-float texture.
-    {
+    if (HasToggleEnabled("use_tint_generator")) {
         wgpu::BindGroupLayout bindGroupLayout = utils::MakeBindGroupLayout(
             device, {{0, wgpu::ShaderStage::Fragment, wgpu::SamplerBindingType::Filtering},
                      {1, wgpu::ShaderStage::Fragment, wgpu::TextureSampleType::UnfilterableFloat}});