[dawn][native] Remove the ExternalTextureExpansionsMap

The last use was in ValidateCompatibilityOfSingleBindingWithLayout
to know if a binding was an ExternalTexture. Instead handle external
textures there as any other kind of bindings.

This makes ExternalTexture correctly validated that they are in the
visibility of the BGL, which wasn't the case previously (so a validation
is added).

As a drive-by the validation of static samplers is also fixed to check
for the visibility in the BGL and that the comparisoness in the shader
matches the BGL.

Bug: 42240282
Change-Id: I08079a5a6351850c27313e6f69a5b10e73f2623f
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/264034
Reviewed-by: Loko Kung <[email protected]>
Commit-Queue: Corentin Wallez <[email protected]>
Reviewed-by: Geoff Lang <[email protected]>

Author: Kangz

src/dawn/native/BindGroupLayoutInternal.cpp

@@ -556,7 +556,6 @@ bool SortBindingsCompare(const BindingInfo& a, const BindingInfo& b) {
 struct ExpandedBindingInfo {
     BindGroupLayoutInternalBase::BindingMap apiBindingMap;
     ityp::vector<BindingIndex, BindingInfo> entries;
-    ExternalTextureBindingExpansionMap externalTextureBindingExpansions;
     std::optional<BindingIndex> dynamicArrayMetadata;
 };
 ExpandedBindingInfo ConvertAndExpandBGLEntries(
@@ -585,7 +584,12 @@ ExpandedBindingInfo ConvertAndExpandBGLEntries(
 
     // Keep track of the BindingNumbers for additional bindings for external textures, it is used
     // below to link the ExternalTextureBindingInfo to the BindingIndex of the additional bindings.
-    ExternalTextureBindingExpansionMap externalTextureExpansions;
+    struct ExternalTextureExpansion {
+        BindingNumber plane0;
+        BindingNumber plane1;
+        BindingNumber params;
+    };
+    absl::flat_hash_map<BindingNumber, ExternalTextureExpansion> externalTextureExpansions;
 
     for (uint32_t i = 0; i < descriptor->entryCount; i++) {
         UnpackedPtr<BindGroupLayoutEntry> entry = Unpack(&descriptor->entries[i]);
@@ -687,7 +691,6 @@ ExpandedBindingInfo ConvertAndExpandBGLEntries(
     }
 
     result.entries = std::move(entries);
-    result.externalTextureBindingExpansions = std::move(externalTextureExpansions);
     return result;
 }
 
@@ -719,8 +722,6 @@ BindGroupLayoutInternalBase::BindGroupLayoutInternalBase(
     ApiObjectBase::UntrackedByDeviceTag tag)
     : ApiObjectBase(device, descriptor->label) {
     ExpandedBindingInfo unpackedBindings = ConvertAndExpandBGLEntries(descriptor);
-    mExternalTextureBindingExpansionMap =
-        std::move(unpackedBindings.externalTextureBindingExpansions);
     mBindingInfo = std::move(unpackedBindings.entries);
     mBindingMap = std::move(unpackedBindings.apiBindingMap);
 
@@ -1055,12 +1056,6 @@ BeginEndRange<BindingIndex> BindGroupLayoutInternalBase::GetInputAttachmentIndic
                  GetBindingTypeEnd(BindingTypeOrder_InputAttachment));
 }
 
-const ExternalTextureBindingExpansionMap&
-BindGroupLayoutInternalBase::GetExternalTextureBindingExpansionMap() const {
-    DAWN_ASSERT(!IsError());
-    return mExternalTextureBindingExpansionMap;
-}
-
 bool BindGroupLayoutInternalBase::NeedsCrossBindingValidation() const {
     DAWN_ASSERT(!IsError());
     return mNeedsCrossBindingValidation;

src/dawn/native/BindGroupLayoutInternal.h

@@ -50,16 +50,6 @@
 #include "dawn/native/dawn_platform.h"
 
 namespace dawn::native {
-// TODO(https://crbug.com/42240282): The expansion information is now stored in
-// ExternalTextureBindingInfo. Remove this structure and the map once backends transition to using
-// ExternalTextureBindingInfo.
-struct ExternalTextureBindingExpansion {
-    BindingNumber plane0;
-    BindingNumber plane1;
-    BindingNumber params;
-};
-using ExternalTextureBindingExpansionMap =
-    absl::flat_hash_map<BindingNumber, ExternalTextureBindingExpansion>;
 
 ResultOrError<UnpackedPtr<BindGroupLayoutDescriptor>> ValidateBindGroupLayoutDescriptor(
     DeviceBase* device,
@@ -187,11 +177,6 @@ class BindGroupLayoutInternalBase : public ApiObjectBase,
     // used to get the stored counts.
     const BindingCounts& GetValidationBindingCounts() const;
 
-    // Used to specify unpacked external texture binding slots when transforming shader modules.
-    // TODO(https://crbug.com/42240282): The expansion information is now stored in
-    // ExternalTextureBindingInfo. Remove this getter once all the backends are updated to use it.
-    const ExternalTextureBindingExpansionMap& GetExternalTextureBindingExpansionMap() const;
-
     uint32_t GetUnexpandedBindingCount() const;
 
     bool NeedsCrossBindingValidation() const;
@@ -256,8 +241,6 @@ class BindGroupLayoutInternalBase : public ApiObjectBase,
     // Map from BindGroupLayoutEntry.binding as BindingNumber to packed indices as BindingIndex.
     // TODO(https://issues.chromium.org/448578977): Use a more optimized map type.
     BindingMap mBindingMap;
-    // Map from the BindingNumber of the ExternalTexture to the BindingNumber of the expansion.
-    ExternalTextureBindingExpansionMap mExternalTextureBindingExpansionMap;
 
     BindingCounts mValidationBindingCounts = {};
     bool mNeedsCrossBindingValidation = false;

src/dawn/native/ShaderModule.cpp

@@ -603,54 +603,30 @@ MaybeError ValidateCompatibilityOfSingleBindingWithLayout(const DeviceBase* devi
                                                           SingleShaderStage entryPointStage,
                                                           BindingNumber bindingNumber,
                                                           const ShaderBindingInfo& shaderInfo) {
+    // Check that the binding exists.
     const BindGroupLayoutInternalBase::BindingMap& layoutBindings = layout->GetBindingMap();
 
-    // An external texture binding found in the shader will later be expanded into multiple
-    // bindings at compile time. This expansion will have already happened in the bgl - so
-    // the shader and bgl will always mismatch at this point. Expansion info is contained in
-    // the bgl object, so we can still verify the bgl used to have an external texture in
-    // the slot corresponding to the shader reflection.
-    if (std::holds_alternative<ExternalTextureBindingInfo>(shaderInfo.bindingInfo)) {
-        // If an external texture binding used to exist in the bgl, it will be found as a
-        // key in the ExternalTextureBindingExpansions map.
-        // TODO(dawn:563): Provide info about the binding types.
-        DAWN_INVALID_IF(!layout->GetExternalTextureBindingExpansionMap().contains(bindingNumber),
-                        "Binding type in the shader (texture_external) doesn't match the "
-                        "type in the layout.");
-
-        return {};
-    }
-
     const auto& bindingIt = layoutBindings.find(bindingNumber);
     DAWN_INVALID_IF(bindingIt == layoutBindings.end(), "Binding doesn't exist in %s.", layout);
 
     APIBindingIndex bindingIndex(bindingIt->second);
     const BindingInfo& layoutInfo = layout->GetAPIBindingInfo(bindingIndex);
 
-    BindingInfoType bindingLayoutType = GetBindingInfoType(layoutInfo);
+    // Check that it is of the same type as in the layout.
     BindingInfoType shaderBindingType = GetShaderBindingType(shaderInfo);
-
-    if (bindingLayoutType == BindingInfoType::StaticSampler) {
-        DAWN_INVALID_IF(shaderBindingType != BindingInfoType::Sampler,
-                        "Binding type in the shader (%s) doesn't match the required type of %s for "
-                        "the %s type in the layout.",
-                        shaderBindingType, BindingInfoType::Sampler, bindingLayoutType);
-        return {};
+    BindingInfoType requiredType = GetBindingInfoType(layoutInfo);
+    if (requiredType == BindingInfoType::StaticSampler) {
+        requiredType = BindingInfoType::Sampler;
     }
-
-    DAWN_INVALID_IF(bindingLayoutType != shaderBindingType,
+    DAWN_INVALID_IF(requiredType != shaderBindingType,
                     "Binding type in the shader (%s) doesn't match the type in the layout (%s).",
-                    shaderBindingType, bindingLayoutType);
-
-    ExternalTextureBindingExpansionMap expansions = layout->GetExternalTextureBindingExpansionMap();
-    DAWN_INVALID_IF(expansions.contains(bindingNumber),
-                    "Binding type (buffer vs. texture vs. sampler vs. external) doesn't "
-                    "match the type in the layout.");
+                    shaderBindingType, requiredType);
 
     DAWN_INVALID_IF((layoutInfo.visibility & StageBit(entryPointStage)) == 0,
                     "Entry point's stage (%s) is not in the binding visibility in the layout (%s).",
                     StageBit(entryPointStage), layoutInfo.visibility);
 
+    // Check the arraySize matches the layout and that the shader has the start of the array.
     DAWN_INVALID_IF(layoutInfo.arraySize < shaderInfo.arraySize,
                     "Binding type in the shader is a binding_array with %u elements but the "
                     "layout only provides %u elements",
@@ -661,6 +637,7 @@ MaybeError ValidateCompatibilityOfSingleBindingWithLayout(const DeviceBase* devi
                     shaderInfo.binding, layoutInfo.indexInArray,
                     uint32_t(layoutInfo.binding) - uint32_t(layoutInfo.indexInArray));
 
+    // Validation specific to each type of binding.
     return MatchVariant(
         shaderInfo.bindingInfo,
         [&](const TextureBindingInfo& bindingInfo) -> MaybeError {
@@ -760,19 +737,26 @@ MaybeError ValidateCompatibilityOfSingleBindingWithLayout(const DeviceBase* devi
             return {};
         },
         [&](const SamplerBindingInfo& bindingInfo) -> MaybeError {
-            const SamplerBindingInfo& bindingLayout =
-                std::get<SamplerBindingInfo>(layoutInfo.bindingLayout);
+            bool comparisonInShader = bindingInfo.type == wgpu::SamplerBindingType::Comparison;
+            bool comparisonInLayout;
+            if (auto* staticBindingLayout =
+                    std::get_if<StaticSamplerBindingInfo>(&layoutInfo.bindingLayout)) {
+                comparisonInLayout = staticBindingLayout->sampler->IsComparison();
+            } else {
+                const SamplerBindingInfo& bindingLayout =
+                    std::get<SamplerBindingInfo>(layoutInfo.bindingLayout);
+                comparisonInLayout = bindingLayout.type == wgpu::SamplerBindingType::Comparison;
+            }
+
             DAWN_INVALID_IF(
-                (bindingLayout.type == wgpu::SamplerBindingType::Comparison) !=
-                    (bindingInfo.type == wgpu::SamplerBindingType::Comparison),
+                comparisonInShader != comparisonInLayout,
                 "The sampler type in the shader (comparison: %u) doesn't match the type in "
                 "the layout (comparison: %u).",
-                bindingInfo.type == wgpu::SamplerBindingType::Comparison,
-                bindingLayout.type == wgpu::SamplerBindingType::Comparison);
+                comparisonInShader, comparisonInLayout);
             return {};
         },
         [](const ExternalTextureBindingInfo&) -> MaybeError {
-            DAWN_UNREACHABLE();
+            // There are no other things to validate for the external textures.
             return {};
         },
         [&](const InputAttachmentBindingInfo& bindingInfo) -> MaybeError {

src/dawn/tests/unittests/validation/BindGroupValidationTests.cpp

@@ -2434,6 +2434,80 @@ TEST_F(BindGroupLayoutWithStaticSamplersValidationTest, StaticSamplerWithArraySi
     ASSERT_DEVICE_ERROR(device.CreateBindGroupLayout(&desc));
 }
 
+// Test that the BGL visibility of a static sampler contain the shader's stage.
+TEST_F(BindGroupLayoutWithStaticSamplersValidationTest, BGLVisibilityContainsShaderStage) {
+    // Set up the static sampler binding but don't create the BGL yet.
+    wgpu::StaticSamplerBindingLayout staticSamplerBinding = {};
+    staticSamplerBinding.sampler = device.CreateSampler();
+
+    wgpu::BindGroupLayoutEntry binding = {};
+    binding.binding = 0;
+    binding.nextInChain = &staticSamplerBinding;
+
+    wgpu::BindGroupLayoutDescriptor desc = {};
+    desc.entryCount = 1;
+    desc.entries = &binding;
+
+    // Set up the compute pipeline but don't create it yet.
+    wgpu::ComputePipelineDescriptor csDesc;
+    csDesc.compute.module = utils::CreateShaderModule(device, R"(
+        @group(0) @binding(0) var s : sampler;
+        @compute @workgroup_size(1) fn main() {
+            _ = s;
+        }
+    )");
+
+    // Success case, the BGL contains compute.
+    binding.visibility = wgpu::ShaderStage::Compute;
+    wgpu::BindGroupLayout bglCompute = device.CreateBindGroupLayout(&desc);
+    csDesc.layout = utils::MakeBasicPipelineLayout(device, &bglCompute);
+    device.CreateComputePipeline(&csDesc);
+
+    // Error case, the BGL doesn't contains compute.
+    binding.visibility = wgpu::ShaderStage::Fragment;
+    wgpu::BindGroupLayout bglFragment = device.CreateBindGroupLayout(&desc);
+    csDesc.layout = utils::MakeBasicPipelineLayout(device, &bglFragment);
+    ASSERT_DEVICE_ERROR(device.CreateComputePipeline(&csDesc));
+}
+
+// Test that the BGL comparisoness for static sampler must match the shader.
+TEST_F(BindGroupLayoutWithStaticSamplersValidationTest, BGLComparisonessMatchesShader) {
+    // Set up the static sampler binding but don't create the BGL yet.
+    wgpu::StaticSamplerBindingLayout staticSamplerBinding = {};
+
+    wgpu::BindGroupLayoutEntry binding = {};
+    binding.binding = 0;
+    binding.visibility = wgpu::ShaderStage::Compute;
+    binding.nextInChain = &staticSamplerBinding;
+
+    wgpu::BindGroupLayoutDescriptor desc = {};
+    desc.entryCount = 1;
+    desc.entries = &binding;
+
+    // Set up the compute pipeline but don't create it yet.
+    wgpu::ComputePipelineDescriptor csDesc;
+    csDesc.compute.module = utils::CreateShaderModule(device, R"(
+        @group(0) @binding(0) var s : sampler;
+        @compute @workgroup_size(1) fn main() {
+            _ = s;
+        }
+    )");
+
+    // Success case, the BGL contains a non-comparison sampler, like the shader.
+    staticSamplerBinding.sampler = device.CreateSampler();
+    wgpu::BindGroupLayout bglCompute = device.CreateBindGroupLayout(&desc);
+    csDesc.layout = utils::MakeBasicPipelineLayout(device, &bglCompute);
+    device.CreateComputePipeline(&csDesc);
+
+    // Error case, the BGL contains a comparison sampler unlike the shader.
+    wgpu::SamplerDescriptor comparisonDesc = {};
+    comparisonDesc.compare = wgpu::CompareFunction::Never;
+    staticSamplerBinding.sampler = device.CreateSampler(&comparisonDesc);
+    wgpu::BindGroupLayout bglFragment = device.CreateBindGroupLayout(&desc);
+    csDesc.layout = utils::MakeBasicPipelineLayout(device, &bglFragment);
+    ASSERT_DEVICE_ERROR(device.CreateComputePipeline(&csDesc));
+}
+
 constexpr uint32_t kBindingSize = 8;
 
 class SetBindGroupValidationTest : public ValidationTest {
@@ -3556,6 +3630,25 @@ TEST_F(BindGroupLayoutCompatibilityTest, ExternalTextureBindGroupLayoutCompatibi
                                                {bgl}));
 }
 
+// Test that the ExternalTexture in the BGL must have visibility that's a superset of the shader.
+TEST_F(BindGroupLayoutCompatibilityTest, ExternalTextureLayoutVisibility) {
+    const char shader[] = R"(
+        @group(0) @binding(0) var myExternalTexture: texture_external;
+        @fragment fn main() {
+            _ = myExternalTexture;
+        })";
+
+    // Success case, visibility in the BGL contains Fragment.
+    wgpu::BindGroupLayout bglFragment = utils::MakeBindGroupLayout(
+        device, {{0, wgpu::ShaderStage::Fragment, &utils::kExternalTextureBindingLayout}});
+    CreateFSRenderPipeline(shader, {bglFragment});
+
+    // Error case, visibility in the BGL doesn't have Fragment.
+    wgpu::BindGroupLayout bglCompute = utils::MakeBindGroupLayout(
+        device, {{0, wgpu::ShaderStage::Compute, &utils::kExternalTextureBindingLayout}});
+    ASSERT_DEVICE_ERROR(CreateFSRenderPipeline(shader, {bglCompute}));
+}
+
 // Test that a BGL is compatible with a pipeline if a binding's array size is at least as big as the
 // shader's binding_array's size.
 TEST_F(BindGroupLayoutCompatibilityTest, ArraySizeCompatibility) {