From 2b0fce7071879b4f81abd0393ea053ad4d34467f Mon Sep 17 00:00:00 2001 From: Brandon Jones Date: Tue, 6 Sep 2022 17:38:03 +0000 Subject: [PATCH] Revert "Implement maxBindingsPerBindGroup limit" This reverts commit 4d67a883b6fd1fe31f7909428044ca88f389491e. Reason for revert: Seems to be causing MSAN issues with the Dawn/Chromium roll? Original change's description: > Implement maxBindingsPerBindGroup limit > > Bug: dawn:1523 > Change-Id: Ifcf83f6836a5d7ed447080ccb033e4163970432e > Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/100706 > Reviewed-by: Kai Ninomiya > Reviewed-by: Austin Eng > Kokoro: Kokoro > Commit-Queue: Brandon Jones # Not skipping CQ checks because original CL landed > 1 day ago. Bug: dawn:1523 Change-Id: I757457089be6af18dfce7f64c0da5d310351cece Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/101401 Kokoro: Kokoro Reviewed-by: Brandon Jones Reviewed-by: Shrek Shao Commit-Queue: Brandon Jones --- dawn.json | 1 - src/dawn/common/Constants.h | 4 +++- src/dawn/native/BindGroupLayout.cpp | 10 +++++----- src/dawn/native/IntegerTypes.h | 2 +- src/dawn/native/Limits.cpp | 1 - src/dawn/native/ShaderModule.cpp | 6 +++--- src/dawn/tests/end2end/BindGroupTests.cpp | 4 ++-- .../validation/BindGroupValidationTests.cpp | 8 ++++---- .../validation/ShaderModuleValidationTests.cpp | 12 ++++++------ 9 files changed, 24 insertions(+), 24 deletions(-) diff --git a/dawn.json b/dawn.json index e2444d8196..8dedc59439 100644 --- a/dawn.json +++ b/dawn.json @@ -1250,7 +1250,6 @@ {"name": "max texture dimension 3D", "type": "uint32_t", "default": "WGPU_LIMIT_U32_UNDEFINED"}, {"name": "max texture array layers", "type": "uint32_t", "default": "WGPU_LIMIT_U32_UNDEFINED"}, {"name": "max bind groups", "type": "uint32_t", "default": "WGPU_LIMIT_U32_UNDEFINED"}, - {"name": "max bindings per bind group", "type": "uint32_t", "default": "WGPU_LIMIT_U32_UNDEFINED"}, {"name": "max dynamic uniform buffers per pipeline layout", "type": "uint32_t", "default": "WGPU_LIMIT_U32_UNDEFINED"}, {"name": "max dynamic storage buffers per pipeline layout", "type": "uint32_t", "default": "WGPU_LIMIT_U32_UNDEFINED"}, {"name": "max sampled textures per shader stage", "type": "uint32_t", "default": "WGPU_LIMIT_U32_UNDEFINED"}, diff --git a/src/dawn/common/Constants.h b/src/dawn/common/Constants.h index 984a584078..a5d20ab54e 100644 --- a/src/dawn/common/Constants.h +++ b/src/dawn/common/Constants.h @@ -18,7 +18,6 @@ #include static constexpr uint32_t kMaxBindGroups = 4u; -static constexpr uint32_t kMaxBindingsPerBindGroup = 640u; static constexpr uint8_t kMaxVertexAttributes = 16u; static constexpr uint8_t kMaxVertexBuffers = 8u; static constexpr uint32_t kMaxVertexBufferArrayStride = 2048u; @@ -63,4 +62,7 @@ static constexpr uint8_t kSampledTexturesPerExternalTexture = 4u; static constexpr uint8_t kSamplersPerExternalTexture = 1u; static constexpr uint8_t kUniformsPerExternalTexture = 1u; +// A spec defined constant but that doesn't have a name. +static constexpr uint32_t kMaxBindingNumber = 65535; + #endif // SRC_DAWN_COMMON_CONSTANTS_H_ diff --git a/src/dawn/native/BindGroupLayout.cpp b/src/dawn/native/BindGroupLayout.cpp index 02f3a3e45a..ae4f071673 100644 --- a/src/dawn/native/BindGroupLayout.cpp +++ b/src/dawn/native/BindGroupLayout.cpp @@ -183,8 +183,8 @@ std::vector ExtractAndExpandBglEntries( std::vector expandedOutput; // When new bgl entries are created, we use binding numbers larger than - // kMaxBindingsPerBindGroup to ensure there are no collisions. - uint32_t nextOpenBindingNumberForNewEntry = kMaxBindingsPerBindGroup; + // kMaxBindingNumber to ensure there are no collisions. + uint32_t nextOpenBindingNumberForNewEntry = kMaxBindingNumber + 1; for (uint32_t i = 0; i < descriptor->entryCount; i++) { const BindGroupLayoutEntry& entry = descriptor->entries[i]; const ExternalTextureBindingLayout* externalTextureBindingLayout = nullptr; @@ -250,9 +250,9 @@ MaybeError ValidateBindGroupLayoutDescriptor(DeviceBase* device, const BindGroupLayoutEntry& entry = descriptor->entries[i]; BindingNumber bindingNumber = BindingNumber(entry.binding); - DAWN_INVALID_IF(bindingNumber >= kMaxBindingsPerBindGroupTyped, - "Binding number (%u) exceeds the maxBindingsPerBindGroup limit (%u).", - uint32_t(bindingNumber), kMaxBindingsPerBindGroup); + DAWN_INVALID_IF(bindingNumber > kMaxBindingNumberTyped, + "Binding number (%u) exceeds the maximum binding number (%u).", + uint32_t(bindingNumber), uint32_t(kMaxBindingNumberTyped)); DAWN_INVALID_IF(bindingsSet.count(bindingNumber) != 0, "On entries[%u]: binding index (%u) was specified by a previous entry.", i, entry.binding); diff --git a/src/dawn/native/IntegerTypes.h b/src/dawn/native/IntegerTypes.h index 18c6ea8a26..0d5c7f89ed 100644 --- a/src/dawn/native/IntegerTypes.h +++ b/src/dawn/native/IntegerTypes.h @@ -23,7 +23,7 @@ namespace dawn::native { // Binding numbers in the shader and BindGroup/BindGroupLayoutDescriptors using BindingNumber = TypedInteger; -constexpr BindingNumber kMaxBindingsPerBindGroupTyped = BindingNumber(kMaxBindingsPerBindGroup); +constexpr BindingNumber kMaxBindingNumberTyped = BindingNumber(kMaxBindingNumber); // Binding numbers get mapped to a packed range of indices using BindingIndex = TypedInteger; diff --git a/src/dawn/native/Limits.cpp b/src/dawn/native/Limits.cpp index d4d686d299..ef285b2a4d 100644 --- a/src/dawn/native/Limits.cpp +++ b/src/dawn/native/Limits.cpp @@ -37,7 +37,6 @@ X(Maximum, maxTextureDimension3D, 2048, 2048) \ X(Maximum, maxTextureArrayLayers, 256, 256) \ X(Maximum, maxBindGroups, 4, 4) \ - X(Maximum, maxBindingsPerBindGroup, 640, 640) \ X(Maximum, maxDynamicUniformBuffersPerPipelineLayout, 8, 8) \ X(Maximum, maxDynamicStorageBuffersPerPipelineLayout, 4, 4) \ X(Maximum, maxSampledTexturesPerShaderStage, 16, 16) \ diff --git a/src/dawn/native/ShaderModule.cpp b/src/dawn/native/ShaderModule.cpp index 1fc7b5b230..6769f7e4ff 100644 --- a/src/dawn/native/ShaderModule.cpp +++ b/src/dawn/native/ShaderModule.cpp @@ -792,9 +792,9 @@ ResultOrError> ReflectEntryPointUsingTint( "The entry-point uses a binding with a group decoration (%u) " "that exceeds the maximum (%u).", resource.bind_group, kMaxBindGroups) || - DelayedInvalidIf(bindingNumber >= kMaxBindingsPerBindGroupTyped, - "Binding number (%u) exceeds the maxBindingsPerBindGroup limit (%u).", - uint32_t(bindingNumber), kMaxBindingsPerBindGroup)) { + DelayedInvalidIf(bindingNumber > kMaxBindingNumberTyped, + "Binding number (%u) exceeds the maximum binding number (%u).", + uint32_t(bindingNumber), uint32_t(kMaxBindingNumberTyped))) { continue; } diff --git a/src/dawn/tests/end2end/BindGroupTests.cpp b/src/dawn/tests/end2end/BindGroupTests.cpp index fc78112638..8a20bcd95c 100644 --- a/src/dawn/tests/end2end/BindGroupTests.cpp +++ b/src/dawn/tests/end2end/BindGroupTests.cpp @@ -1254,7 +1254,7 @@ TEST_P(BindGroupTests, ArbitraryBindingNumbers) { color : vec4 } - @group(0) @binding(553) var ubo1 : Ubo; + @group(0) @binding(953) var ubo1 : Ubo; @group(0) @binding(47) var ubo2 : Ubo; @group(0) @binding(111) var ubo3 : Ubo; @@ -1295,7 +1295,7 @@ TEST_P(BindGroupTests, ArbitraryBindingNumbers) { }; utils::BindingInitializationHelper bindings[] = { - {553, color1, 0, 4 * sizeof(float)}, // + {953, color1, 0, 4 * sizeof(float)}, // {47, color2, 0, 4 * sizeof(float)}, // {111, color3, 0, 4 * sizeof(float)}, // }; diff --git a/src/dawn/tests/unittests/validation/BindGroupValidationTests.cpp b/src/dawn/tests/unittests/validation/BindGroupValidationTests.cpp index 4f0132cd23..112c6691c9 100644 --- a/src/dawn/tests/unittests/validation/BindGroupValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/BindGroupValidationTests.cpp @@ -1017,14 +1017,14 @@ TEST_F(BindGroupLayoutValidationTest, BindGroupLayoutStorageBindingsInVertexShad // Tests setting that bind group layout bindings numbers may be very large. TEST_F(BindGroupLayoutValidationTest, BindGroupLayoutEntryMax) { - // Check that up to kMaxBindingsPerBindGroup-1 is valid. - utils::MakeBindGroupLayout(device, {{kMaxBindingsPerBindGroup - 1, wgpu::ShaderStage::Vertex, - wgpu::BufferBindingType::Uniform}}); + // Check that up to kMaxBindingNumber is valid. + utils::MakeBindGroupLayout( + device, {{kMaxBindingNumber, wgpu::ShaderStage::Vertex, wgpu::BufferBindingType::Uniform}}); // But after is an error. ASSERT_DEVICE_ERROR(utils::MakeBindGroupLayout( device, - {{kMaxBindingsPerBindGroup, wgpu::ShaderStage::Vertex, wgpu::BufferBindingType::Uniform}})); + {{kMaxBindingNumber + 1, wgpu::ShaderStage::Vertex, wgpu::BufferBindingType::Uniform}})); } // This test verifies that the BindGroupLayout bindings are correctly validated, even if the diff --git a/src/dawn/tests/unittests/validation/ShaderModuleValidationTests.cpp b/src/dawn/tests/unittests/validation/ShaderModuleValidationTests.cpp index 33b80c45cf..17e25f96ee 100644 --- a/src/dawn/tests/unittests/validation/ShaderModuleValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/ShaderModuleValidationTests.cpp @@ -550,25 +550,25 @@ struct Buf { })")); } -// Test that @binding must be less then kMaxBindingsPerBindGroup +// Test that @binding must be less then kMaxBindingNumber TEST_F(ShaderModuleValidationTest, MaxBindingNumber) { - static_assert(kMaxBindingsPerBindGroup == 640); + static_assert(kMaxBindingNumber == 65535); wgpu::ComputePipelineDescriptor desc; desc.compute.entryPoint = "main"; - // kMaxBindingsPerBindGroup-1 is valid. + // kMaxBindingNumber is valid. desc.compute.module = utils::CreateShaderModule(device, R"( - @group(0) @binding(639) var s : sampler; + @group(0) @binding(65535) var s : sampler; @compute @workgroup_size(1) fn main() { _ = s; } )"); device.CreateComputePipeline(&desc); - // kMaxBindingsPerBindGroup is an error + // kMaxBindingNumber + 1 is an error desc.compute.module = utils::CreateShaderModule(device, R"( - @group(0) @binding(640) var s : sampler; + @group(0) @binding(65536) var s : sampler; @compute @workgroup_size(1) fn main() { _ = s; }