From a80993da44e02ec76e1a8de85b1613e743e97e1b Mon Sep 17 00:00:00 2001 From: Austin Eng Date: Fri, 20 Mar 2020 21:56:30 +0000 Subject: [PATCH] Support and pack unbounded binding numbers in the BGL Also fixes a bug where we weren't validating duplicating bindings in the shader, and where dynamic offset validation could be incorrectly fetching the wrong bindings. Bug: dawn:354 Change-Id: I93178c34eb4d43119e8b9de5738ae4596e9277cd Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/17240 Reviewed-by: Corentin Wallez Reviewed-by: Kai Ninomiya Commit-Queue: Austin Eng --- src/dawn_native/BindGroup.cpp | 59 +++--- src/dawn_native/BindGroup.h | 6 +- .../BindGroupAndStorageBarrierTracker.h | 17 +- src/dawn_native/BindGroupLayout.cpp | 168 +++++++++++++----- src/dawn_native/BindGroupLayout.h | 26 ++- src/dawn_native/Device.h | 3 +- src/dawn_native/IntegerTypes.h | 33 ++++ src/dawn_native/PipelineLayout.cpp | 39 ++-- src/dawn_native/ProgrammablePassEncoder.cpp | 40 +++-- src/dawn_native/ShaderModule.cpp | 65 ++++--- src/dawn_native/ShaderModule.h | 6 +- src/dawn_native/d3d12/BindGroupD3D12.cpp | 4 +- .../d3d12/BindGroupLayoutD3D12.cpp | 22 +-- src/dawn_native/d3d12/ShaderModuleD3D12.cpp | 32 ++-- src/dawn_native/metal/CommandBufferMTL.mm | 6 +- src/dawn_native/metal/PipelineLayoutMTL.mm | 19 +- src/dawn_native/metal/ShaderModuleMTL.mm | 19 +- src/dawn_native/opengl/CommandBufferGL.cpp | 6 +- src/dawn_native/opengl/PipelineGL.cpp | 19 +- src/dawn_native/opengl/PipelineLayoutGL.cpp | 20 +-- src/dawn_native/opengl/ShaderModuleGL.cpp | 27 ++- src/dawn_native/vulkan/BindGroupLayoutVk.cpp | 19 +- src/dawn_native/vulkan/BindGroupVk.cpp | 9 +- src/dawn_native/vulkan/CommandBufferVk.cpp | 6 +- src/tests/end2end/BindGroupTests.cpp | 97 ++++++++++ .../validation/BindGroupValidationTests.cpp | 72 +++++--- .../StorageTextureValidationTests.cpp | 2 +- 27 files changed, 561 insertions(+), 280 deletions(-) create mode 100644 src/dawn_native/IntegerTypes.h diff --git a/src/dawn_native/BindGroup.cpp b/src/dawn_native/BindGroup.cpp index 9e64c46720..32906a1fca 100644 --- a/src/dawn_native/BindGroup.cpp +++ b/src/dawn_native/BindGroup.cpp @@ -118,23 +118,22 @@ namespace dawn_native { const BindGroupLayoutBase::LayoutBindingInfo& layoutInfo = descriptor->layout->GetBindingInfo(); - if (descriptor->bindingCount != layoutInfo.mask.count()) { + const BindGroupLayoutBase::BindingMap& bindingMap = descriptor->layout->GetBindingMap(); + + if (descriptor->bindingCount != bindingMap.size()) { return DAWN_VALIDATION_ERROR("numBindings mismatch"); } std::bitset bindingsSet; for (uint32_t i = 0; i < descriptor->bindingCount; ++i) { const BindGroupBinding& binding = descriptor->bindings[i]; - uint32_t bindingIndex = binding.binding; - // Check that we can set this binding. - if (bindingIndex >= kMaxBindingsPerGroup) { - return DAWN_VALIDATION_ERROR("binding index too high"); - } - - if (!layoutInfo.mask[bindingIndex]) { + const auto& it = bindingMap.find(BindingNumber(binding.binding)); + if (it == bindingMap.end()) { return DAWN_VALIDATION_ERROR("setting non-existent binding"); } + BindingIndex bindingIndex = it->second; + ASSERT(bindingIndex < layoutInfo.bindingCount); if (bindingsSet[bindingIndex]) { return DAWN_VALIDATION_ERROR("binding set twice"); @@ -176,7 +175,7 @@ namespace dawn_native { // - Each binding must be set at most once // // We don't validate the equality because it wouldn't be possible to cover it with a test. - ASSERT(bindingsSet == layoutInfo.mask); + ASSERT(bindingsSet.count() == bindingMap.size()); return {}; } @@ -189,7 +188,7 @@ namespace dawn_native { : ObjectBase(device), mLayout(descriptor->layout), mBindingData(mLayout->ComputeBindingDataPointers(bindingDataStart)) { - for (uint32_t i = 0; i < mLayout->GetBindingCount(); ++i) { + for (BindingIndex i = 0; i < mLayout->GetBindingCount(); ++i) { // TODO(enga): Shouldn't be needed when bindings are tightly packed. // This is to fill Ref holes with nullptrs. new (&mBindingData.bindings[i]) Ref(); @@ -198,7 +197,8 @@ namespace dawn_native { for (uint32_t i = 0; i < descriptor->bindingCount; ++i) { const BindGroupBinding& binding = descriptor->bindings[i]; - uint32_t bindingIndex = binding.binding; + BindingIndex bindingIndex = + descriptor->layout->GetBindingIndex(BindingNumber(binding.binding)); ASSERT(bindingIndex < mLayout->GetBindingCount()); // Only a single binding type should be set, so once we found it we can skip to the @@ -231,7 +231,7 @@ namespace dawn_native { BindGroupBase::~BindGroupBase() { if (mLayout) { ASSERT(!IsError()); - for (uint32_t i = 0; i < mLayout->GetBindingCount(); ++i) { + for (BindingIndex i = 0; i < mLayout->GetBindingCount(); ++i) { mBindingData.bindings[i].~Ref(); } } @@ -251,33 +251,30 @@ namespace dawn_native { return mLayout.Get(); } - BufferBinding BindGroupBase::GetBindingAsBufferBinding(size_t binding) { + BufferBinding BindGroupBase::GetBindingAsBufferBinding(BindingIndex bindingIndex) { ASSERT(!IsError()); - ASSERT(binding < kMaxBindingsPerGroup); - ASSERT(mLayout->GetBindingInfo().mask[binding]); - ASSERT(mLayout->GetBindingInfo().types[binding] == wgpu::BindingType::UniformBuffer || - mLayout->GetBindingInfo().types[binding] == wgpu::BindingType::StorageBuffer || - mLayout->GetBindingInfo().types[binding] == + ASSERT(bindingIndex < mLayout->GetBindingCount()); + ASSERT(mLayout->GetBindingInfo().types[bindingIndex] == wgpu::BindingType::UniformBuffer || + mLayout->GetBindingInfo().types[bindingIndex] == wgpu::BindingType::StorageBuffer || + mLayout->GetBindingInfo().types[bindingIndex] == wgpu::BindingType::ReadonlyStorageBuffer); - BufferBase* buffer = static_cast(mBindingData.bindings[binding].Get()); - return {buffer, mBindingData.bufferData[binding].offset, - mBindingData.bufferData[binding].size}; + BufferBase* buffer = static_cast(mBindingData.bindings[bindingIndex].Get()); + return {buffer, mBindingData.bufferData[bindingIndex].offset, + mBindingData.bufferData[bindingIndex].size}; } - SamplerBase* BindGroupBase::GetBindingAsSampler(size_t binding) { + SamplerBase* BindGroupBase::GetBindingAsSampler(BindingIndex bindingIndex) { ASSERT(!IsError()); - ASSERT(binding < kMaxBindingsPerGroup); - ASSERT(mLayout->GetBindingInfo().mask[binding]); - ASSERT(mLayout->GetBindingInfo().types[binding] == wgpu::BindingType::Sampler); - return static_cast(mBindingData.bindings[binding].Get()); + ASSERT(bindingIndex < mLayout->GetBindingCount()); + ASSERT(mLayout->GetBindingInfo().types[bindingIndex] == wgpu::BindingType::Sampler); + return static_cast(mBindingData.bindings[bindingIndex].Get()); } - TextureViewBase* BindGroupBase::GetBindingAsTextureView(size_t binding) { + TextureViewBase* BindGroupBase::GetBindingAsTextureView(BindingIndex bindingIndex) { ASSERT(!IsError()); - ASSERT(binding < kMaxBindingsPerGroup); - ASSERT(mLayout->GetBindingInfo().mask[binding]); - ASSERT(mLayout->GetBindingInfo().types[binding] == wgpu::BindingType::SampledTexture); - return static_cast(mBindingData.bindings[binding].Get()); + ASSERT(bindingIndex < mLayout->GetBindingCount()); + ASSERT(mLayout->GetBindingInfo().types[bindingIndex] == wgpu::BindingType::SampledTexture); + return static_cast(mBindingData.bindings[bindingIndex].Get()); } } // namespace dawn_native diff --git a/src/dawn_native/BindGroup.h b/src/dawn_native/BindGroup.h index 68322b2c57..becb39ee75 100644 --- a/src/dawn_native/BindGroup.h +++ b/src/dawn_native/BindGroup.h @@ -46,9 +46,9 @@ namespace dawn_native { static BindGroupBase* MakeError(DeviceBase* device); BindGroupLayoutBase* GetLayout(); - BufferBinding GetBindingAsBufferBinding(size_t binding); - SamplerBase* GetBindingAsSampler(size_t binding); - TextureViewBase* GetBindingAsTextureView(size_t binding); + BufferBinding GetBindingAsBufferBinding(BindingIndex bindingIndex); + SamplerBase* GetBindingAsSampler(BindingIndex bindingIndex); + TextureViewBase* GetBindingAsTextureView(BindingIndex bindingIndex); protected: // To save memory, the size of a bind group is dynamically determined and the bind group is diff --git a/src/dawn_native/BindGroupAndStorageBarrierTracker.h b/src/dawn_native/BindGroupAndStorageBarrierTracker.h index 06fc268a98..975e96f1b0 100644 --- a/src/dawn_native/BindGroupAndStorageBarrierTracker.h +++ b/src/dawn_native/BindGroupAndStorageBarrierTracker.h @@ -38,15 +38,16 @@ namespace dawn_native { mBuffersNeedingBarrier[index] = {}; const BindGroupLayoutBase* layout = bindGroup->GetLayout(); - const auto& info = layout->GetBindingInfo(); + const BindGroupLayoutBase::LayoutBindingInfo& info = layout->GetBindingInfo(); - for (uint32_t binding : IterateBitSet(info.mask)) { - if ((info.visibilities[binding] & wgpu::ShaderStage::Compute) == 0) { + for (BindingIndex bindingIndex = 0; bindingIndex < info.bindingCount; + ++bindingIndex) { + if ((info.visibilities[bindingIndex] & wgpu::ShaderStage::Compute) == 0) { continue; } - mBindingTypes[index][binding] = info.types[binding]; - switch (info.types[binding]) { + mBindingTypes[index][bindingIndex] = info.types[bindingIndex]; + switch (info.types[bindingIndex]) { case wgpu::BindingType::UniformBuffer: case wgpu::BindingType::ReadonlyStorageBuffer: case wgpu::BindingType::Sampler: @@ -55,9 +56,9 @@ namespace dawn_native { break; case wgpu::BindingType::StorageBuffer: - mBuffersNeedingBarrier[index].set(binding); - mBuffers[index][binding] = - bindGroup->GetBindingAsBufferBinding(binding).buffer; + mBuffersNeedingBarrier[index].set(bindingIndex); + mBuffers[index][bindingIndex] = + bindGroup->GetBindingAsBufferBinding(bindingIndex).buffer; break; case wgpu::BindingType::StorageTexture: diff --git a/src/dawn_native/BindGroupLayout.cpp b/src/dawn_native/BindGroupLayout.cpp index 622ed02c85..57730322b0 100644 --- a/src/dawn_native/BindGroupLayout.cpp +++ b/src/dawn_native/BindGroupLayout.cpp @@ -14,13 +14,14 @@ #include "dawn_native/BindGroupLayout.h" -#include - #include "common/BitSetIterator.h" #include "common/HashUtils.h" #include "dawn_native/Device.h" #include "dawn_native/ValidationUtils_autogen.h" +#include +#include + namespace dawn_native { MaybeError ValidateBindingTypeWithShaderStageVisibility( @@ -92,11 +93,13 @@ namespace dawn_native { return DAWN_VALIDATION_ERROR("nextInChain must be nullptr"); } - std::bitset bindingsSet; + std::set bindingsSet; uint32_t dynamicUniformBufferCount = 0; uint32_t dynamicStorageBufferCount = 0; - for (uint32_t i = 0; i < descriptor->bindingCount; ++i) { + for (BindingIndex i = 0; i < descriptor->bindingCount; ++i) { const BindGroupLayoutBinding& binding = descriptor->bindings[i]; + BindingNumber bindingNumber = BindingNumber(binding.binding); + DAWN_TRY(ValidateShaderStage(binding.visibility)); DAWN_TRY(ValidateBindingType(binding.type)); DAWN_TRY(ValidateTextureComponentType(binding.textureComponentType)); @@ -105,10 +108,7 @@ namespace dawn_native { DAWN_TRY(ValidateTextureViewDimension(binding.textureDimension)); } - if (binding.binding >= kMaxBindingsPerGroup) { - return DAWN_VALIDATION_ERROR("some binding index exceeds the maximum value"); - } - if (bindingsSet[binding.binding]) { + if (bindingsSet.count(bindingNumber) != 0) { return DAWN_VALIDATION_ERROR("some binding index was specified more than once"); } @@ -147,7 +147,7 @@ namespace dawn_native { "BindGroupLayoutBinding::multisampled must be false (for now)"); } - bindingsSet.set(binding.binding); + bindingsSet.insert(bindingNumber); } if (dynamicUniformBufferCount > kMaxDynamicUniformBufferCount) { @@ -165,12 +165,12 @@ namespace dawn_native { namespace { size_t HashBindingInfo(const BindGroupLayoutBase::LayoutBindingInfo& info) { - size_t hash = Hash(info.mask); + size_t hash = 0; HashCombine(&hash, info.hasDynamicOffset, info.multisampled); - for (uint32_t binding : IterateBitSet(info.mask)) { - HashCombine(&hash, info.visibilities[binding], info.types[binding], - info.textureComponentTypes[binding], info.textureDimensions[binding]); + for (BindingIndex i = 0; i < info.bindingCount; ++i) { + HashCombine(&hash, info.visibilities[i], info.types[i], + info.textureComponentTypes[i], info.textureDimensions[i]); } return hash; @@ -178,22 +178,83 @@ namespace dawn_native { bool operator==(const BindGroupLayoutBase::LayoutBindingInfo& a, const BindGroupLayoutBase::LayoutBindingInfo& b) { - if (a.mask != b.mask || a.hasDynamicOffset != b.hasDynamicOffset || + if (a.bindingCount != b.bindingCount || a.hasDynamicOffset != b.hasDynamicOffset || a.multisampled != b.multisampled) { return false; } - for (uint32_t binding : IterateBitSet(a.mask)) { - if ((a.visibilities[binding] != b.visibilities[binding]) || - (a.types[binding] != b.types[binding]) || - (a.textureComponentTypes[binding] != b.textureComponentTypes[binding]) || - (a.textureDimensions[binding] != b.textureDimensions[binding])) { + for (BindingIndex i = 0; i < a.bindingCount; ++i) { + if ((a.visibilities[i] != b.visibilities[i]) || (a.types[i] != b.types[i]) || + (a.textureComponentTypes[i] != b.textureComponentTypes[i]) || + (a.textureDimensions[i] != b.textureDimensions[i])) { return false; } } return true; } + + bool SortBindingsCompare(const BindGroupLayoutBinding& a, const BindGroupLayoutBinding& b) { + if (a.hasDynamicOffset != b.hasDynamicOffset) { + // Buffers with dynamic offsets should come before those without. + // This makes it easy to iterate over the dynamic buffer bindings + // [0, dynamicBufferCount) during validation. + return a.hasDynamicOffset > b.hasDynamicOffset; + } + if (a.type != b.type) { + // Buffers have smaller type enums. They should be placed first. + return a.type < b.type; + } + if (a.visibility != b.visibility) { + return a.visibility < b.visibility; + } + if (a.multisampled != b.multisampled) { + return a.multisampled < b.multisampled; + } + if (a.textureDimension != b.textureDimension) { + return a.textureDimension < b.textureDimension; + } + if (a.textureComponentType != b.textureComponentType) { + return a.textureComponentType < b.textureComponentType; + } + if (a.storageTextureFormat != b.storageTextureFormat) { + return a.storageTextureFormat < b.storageTextureFormat; + } + return false; + } + + // This is a utility function to help ASSERT that the BGL-binding comparator places buffers + // first. + bool CheckBufferBindingsFirst(const BindGroupLayoutBinding* bindings, size_t count) { + ASSERT(count <= kMaxBindingsPerGroup); + + BindingIndex lastBufferIndex = 0; + BindingIndex firstNonBufferIndex = std::numeric_limits::max(); + for (BindingIndex i = 0; i < count; ++i) { + switch (bindings[i].type) { + case wgpu::BindingType::UniformBuffer: + case wgpu::BindingType::StorageBuffer: + case wgpu::BindingType::ReadonlyStorageBuffer: + lastBufferIndex = std::max(i, lastBufferIndex); + break; + case wgpu::BindingType::SampledTexture: + case wgpu::BindingType::Sampler: + case wgpu::BindingType::StorageTexture: + case wgpu::BindingType::ReadonlyStorageTexture: + case wgpu::BindingType::WriteonlyStorageTexture: + firstNonBufferIndex = std::min(i, firstNonBufferIndex); + break; + default: + UNREACHABLE(); + break; + } + } + + // If there are no buffers, then |lastBufferIndex| is initialized to 0 and + // |firstNonBufferIndex| gets set to 0. + return firstNonBufferIndex >= lastBufferIndex; + } + } // namespace // BindGroupLayoutBase @@ -201,34 +262,40 @@ namespace dawn_native { BindGroupLayoutBase::BindGroupLayoutBase(DeviceBase* device, const BindGroupLayoutDescriptor* descriptor) : CachedObject(device) { - for (uint32_t i = 0; i < descriptor->bindingCount; ++i) { - auto& binding = descriptor->bindings[i]; + mBindingInfo.bindingCount = descriptor->bindingCount; - uint32_t index = binding.binding; - mBindingInfo.visibilities[index] = binding.visibility; - mBindingInfo.types[index] = binding.type; - mBindingInfo.textureComponentTypes[index] = binding.textureComponentType; + std::vector sortedBindings( + descriptor->bindings, descriptor->bindings + descriptor->bindingCount); + + std::sort(sortedBindings.begin(), sortedBindings.end(), SortBindingsCompare); + ASSERT(CheckBufferBindingsFirst(sortedBindings.data(), sortedBindings.size())); + + for (BindingIndex i = 0; i < mBindingInfo.bindingCount; ++i) { + const BindGroupLayoutBinding& binding = sortedBindings[i]; + mBindingInfo.types[i] = binding.type; + mBindingInfo.visibilities[i] = binding.visibility; + mBindingInfo.textureComponentTypes[i] = binding.textureComponentType; - // TODO(enga): This is a greedy computation because there may be holes in bindings. - // Fix this when we pack bindings. - mBindingCount = std::max(mBindingCount, index + 1); switch (binding.type) { case wgpu::BindingType::UniformBuffer: case wgpu::BindingType::StorageBuffer: case wgpu::BindingType::ReadonlyStorageBuffer: - mBufferCount = std::max(mBufferCount, index + 1); + // Buffers must be contiguously packed at the start of the binding info. + ASSERT(mBufferCount == i); + ++mBufferCount; break; default: break; } if (binding.textureDimension == wgpu::TextureViewDimension::Undefined) { - mBindingInfo.textureDimensions[index] = wgpu::TextureViewDimension::e2D; + mBindingInfo.textureDimensions[i] = wgpu::TextureViewDimension::e2D; } else { - mBindingInfo.textureDimensions[index] = binding.textureDimension; + mBindingInfo.textureDimensions[i] = binding.textureDimension; } + if (binding.hasDynamicOffset) { - mBindingInfo.hasDynamicOffset.set(index); + mBindingInfo.hasDynamicOffset.set(i); switch (binding.type) { case wgpu::BindingType::UniformBuffer: ++mDynamicUniformBufferCount; @@ -246,11 +313,10 @@ namespace dawn_native { break; } } + mBindingInfo.multisampled.set(i, binding.multisampled); - mBindingInfo.multisampled.set(index, binding.multisampled); - - ASSERT(!mBindingInfo.mask[index]); - mBindingInfo.mask.set(index); + const auto& it = mBindingMap.emplace(BindingNumber(binding.binding), i); + ASSERT(it.second); } } @@ -275,20 +341,38 @@ namespace dawn_native { return mBindingInfo; } + const BindGroupLayoutBase::BindingMap& BindGroupLayoutBase::GetBindingMap() const { + ASSERT(!IsError()); + return mBindingMap; + } + + BindingIndex BindGroupLayoutBase::GetBindingIndex(BindingNumber bindingNumber) const { + ASSERT(!IsError()); + const auto& it = mBindingMap.find(bindingNumber); + ASSERT(it != mBindingMap.end()); + return it->second; + } + size_t BindGroupLayoutBase::HashFunc::operator()(const BindGroupLayoutBase* bgl) const { - return HashBindingInfo(bgl->mBindingInfo); + size_t hash = HashBindingInfo(bgl->mBindingInfo); + // std::map is sorted by key, so two BGLs constructed in different orders + // will still hash the same. + for (const auto& it : bgl->mBindingMap) { + HashCombine(&hash, it.first, it.second); + } + return hash; } bool BindGroupLayoutBase::EqualityFunc::operator()(const BindGroupLayoutBase* a, const BindGroupLayoutBase* b) const { - return a->mBindingInfo == b->mBindingInfo; + return a->mBindingInfo == b->mBindingInfo && a->mBindingMap == b->mBindingMap; } - uint32_t BindGroupLayoutBase::GetBindingCount() const { - return mBindingCount; + BindingIndex BindGroupLayoutBase::GetBindingCount() const { + return mBindingInfo.bindingCount; } - uint32_t BindGroupLayoutBase::GetDynamicBufferCount() const { + BindingIndex BindGroupLayoutBase::GetDynamicBufferCount() const { return mDynamicStorageBufferCount + mDynamicUniformBufferCount; } @@ -305,7 +389,7 @@ namespace dawn_native { // | --- offsets + sizes -------------| --------------- Ref ----------| size_t objectPointerStart = mBufferCount * sizeof(BufferBindingData); ASSERT(IsAligned(objectPointerStart, alignof(Ref))); - return objectPointerStart + mBindingCount * sizeof(Ref); + return objectPointerStart + mBindingInfo.bindingCount * sizeof(Ref); } BindGroupLayoutBase::BindingDataPointers BindGroupLayoutBase::ComputeBindingDataPointers( diff --git a/src/dawn_native/BindGroupLayout.h b/src/dawn_native/BindGroupLayout.h index 7455b58cbe..ed96c27260 100644 --- a/src/dawn_native/BindGroupLayout.h +++ b/src/dawn_native/BindGroupLayout.h @@ -21,11 +21,13 @@ #include "dawn_native/CachedObject.h" #include "dawn_native/Error.h" #include "dawn_native/Forward.h" +#include "dawn_native/IntegerTypes.h" #include "dawn_native/dawn_platform.h" #include #include +#include namespace dawn_native { @@ -40,6 +42,9 @@ namespace dawn_native { wgpu::BindingType bindingType, wgpu::TextureFormat storageTextureFormat); + // Bindings are specified as a |BindingNumber| in the BindGroupLayoutDescriptor. + // These numbers may be arbitrary and sparse. Internally, Dawn packs these numbers + // into a packed range of |BindingIndex| integers. class BindGroupLayoutBase : public CachedObject { public: BindGroupLayoutBase(DeviceBase* device, const BindGroupLayoutDescriptor* descriptor); @@ -54,9 +59,15 @@ namespace dawn_native { std::array textureDimensions; std::bitset hasDynamicOffset; std::bitset multisampled; - std::bitset mask; + BindingIndex bindingCount; }; + + // A map from the BindingNumber to its packed BindingIndex. + using BindingMap = std::map; + const LayoutBindingInfo& GetBindingInfo() const; + const BindingMap& GetBindingMap() const; + BindingIndex GetBindingIndex(BindingNumber bindingNumber) const; // Functors necessary for the unordered_set-based cache. struct HashFunc { @@ -66,8 +77,9 @@ namespace dawn_native { bool operator()(const BindGroupLayoutBase* a, const BindGroupLayoutBase* b) const; }; - uint32_t GetBindingCount() const; - uint32_t GetDynamicBufferCount() const; + BindingIndex GetBindingCount() const; + // Returns |BindingIndex| because dynamic buffers are packed at the front. + BindingIndex GetDynamicBufferCount() const; uint32_t GetDynamicUniformBufferCount() const; uint32_t GetDynamicStorageBufferCount() const; @@ -104,11 +116,13 @@ namespace dawn_native { private: BindGroupLayoutBase(DeviceBase* device, ObjectBase::ErrorTag tag); - LayoutBindingInfo mBindingInfo; - uint32_t mBindingCount = 0; - uint32_t mBufferCount = 0; + BindingIndex mBufferCount = 0; // |BindingIndex| because buffers are packed at the front. uint32_t mDynamicUniformBufferCount = 0; uint32_t mDynamicStorageBufferCount = 0; + LayoutBindingInfo mBindingInfo; + + // Map from BindGroupLayoutEntry.binding to packed indices. + BindingMap mBindingMap; }; } // namespace dawn_native diff --git a/src/dawn_native/Device.h b/src/dawn_native/Device.h index 6474a642d1..235ccd222e 100644 --- a/src/dawn_native/Device.h +++ b/src/dawn_native/Device.h @@ -32,10 +32,11 @@ namespace dawn_native { class AdapterBase; class AttachmentState; class AttachmentStateBlueprint; + class BindGroupLayoutBase; + class DynamicUploader; class ErrorScope; class ErrorScopeTracker; class FenceSignalTracker; - class DynamicUploader; class StagingBufferBase; class DeviceBase { diff --git a/src/dawn_native/IntegerTypes.h b/src/dawn_native/IntegerTypes.h new file mode 100644 index 0000000000..04559353dd --- /dev/null +++ b/src/dawn_native/IntegerTypes.h @@ -0,0 +1,33 @@ +// Copyright 2020 The Dawn Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef DAWNNATIVE_INTEGERTYPES_H_ +#define DAWNNATIVE_INTEGERTYPES_H_ + +#include + +namespace dawn_native { + + // TODO(enga): Can we have strongly typed integers so you can't convert between them + // by accident? And also range-assertions (ex. kMaxBindingsPerGroup) in Debug? + + // Binding numbers in the shader and BindGroup/BindGroupLayoutDescriptors + using BindingNumber = uint32_t; + + // Binding numbers get mapped to a packed range of indices + using BindingIndex = uint32_t; + +} // namespace dawn_native + +#endif // DAWNNATIVE_INTEGERTYPES_H_ diff --git a/src/dawn_native/PipelineLayout.cpp b/src/dawn_native/PipelineLayout.cpp index f72bbdaa9c..a7c74a7a1d 100644 --- a/src/dawn_native/PipelineLayout.cpp +++ b/src/dawn_native/PipelineLayout.cpp @@ -130,11 +130,8 @@ namespace dawn_native { std::array, kMaxBindGroups> bindingData = {}; - // Bitsets of used bindings - std::array, kMaxBindGroups> usedBindings = {}; - - // A flat map of bindings to the index in |bindingData| - std::array, kMaxBindGroups> usedBindingsMap = {}; + // A map of bindings to the index in |bindingData| + std::array, kMaxBindGroups> usedBindingsMap = {}; // A counter of how many bindings we've populated in |bindingData| std::array bindingCounts = {}; @@ -145,18 +142,16 @@ namespace dawn_native { const ShaderModuleBase::ModuleBindingInfo& info = module->GetBindingInfo(); for (uint32_t group = 0; group < info.size(); ++group) { - for (uint32_t binding = 0; binding < info[group].size(); ++binding) { - const ShaderModuleBase::BindingInfo& bindingInfo = info[group][binding]; - if (!bindingInfo.used) { - continue; - } + for (const auto& it : info[group]) { + BindingNumber bindingNumber = it.first; + const ShaderModuleBase::BindingInfo& bindingInfo = it.second; if (bindingInfo.multisampled) { return DAWN_VALIDATION_ERROR("Multisampled textures not supported (yet)"); } BindGroupLayoutBinding bindingSlot; - bindingSlot.binding = binding; + bindingSlot.binding = bindingNumber; DAWN_TRY(ValidateBindingTypeWithShaderStageVisibility( bindingInfo.type, StageBit(module->GetExecutionModel()))); @@ -174,22 +169,24 @@ namespace dawn_native { Format::FormatTypeToTextureComponentType(bindingInfo.textureComponentType); bindingSlot.storageTextureFormat = bindingInfo.storageTextureFormat; - if (usedBindings[group][binding]) { - if (bindingSlot == bindingData[group][usedBindingsMap[group][binding]]) { - // Already used and the data is the same. Continue. - continue; - } else { - return DAWN_VALIDATION_ERROR( - "Duplicate binding in default pipeline layout initialization not " - "compatible with previous declaration"); + { + const auto& it = usedBindingsMap[group].find(bindingNumber); + if (it != usedBindingsMap[group].end()) { + if (bindingSlot == bindingData[group][it->second]) { + // Already used and the data is the same. Continue. + continue; + } else { + return DAWN_VALIDATION_ERROR( + "Duplicate binding in default pipeline layout initialization " + "not compatible with previous declaration"); + } } } uint32_t currentBindingCount = bindingCounts[group]; bindingData[group][currentBindingCount] = bindingSlot; - usedBindingsMap[group][binding] = currentBindingCount; - usedBindings[group].set(binding); + usedBindingsMap[group][bindingNumber] = currentBindingCount; bindingCounts[group]++; diff --git a/src/dawn_native/ProgrammablePassEncoder.cpp b/src/dawn_native/ProgrammablePassEncoder.cpp index a68c58fb72..fd03d7040e 100644 --- a/src/dawn_native/ProgrammablePassEncoder.cpp +++ b/src/dawn_native/ProgrammablePassEncoder.cpp @@ -29,29 +29,31 @@ namespace dawn_native { namespace { void TrackBindGroupResourceUsage(PassResourceUsageTracker* usageTracker, BindGroupBase* group) { - const auto& layoutInfo = group->GetLayout()->GetBindingInfo(); - - for (uint32_t i : IterateBitSet(layoutInfo.mask)) { - wgpu::BindingType type = layoutInfo.types[i]; + const BindGroupLayoutBase::LayoutBindingInfo& layoutInfo = + group->GetLayout()->GetBindingInfo(); + for (BindingIndex bindingIndex = 0; bindingIndex < layoutInfo.bindingCount; + ++bindingIndex) { + wgpu::BindingType type = layoutInfo.types[bindingIndex]; switch (type) { case wgpu::BindingType::UniformBuffer: { - BufferBase* buffer = group->GetBindingAsBufferBinding(i).buffer; + BufferBase* buffer = group->GetBindingAsBufferBinding(bindingIndex).buffer; usageTracker->BufferUsedAs(buffer, wgpu::BufferUsage::Uniform); } break; case wgpu::BindingType::StorageBuffer: { - BufferBase* buffer = group->GetBindingAsBufferBinding(i).buffer; + BufferBase* buffer = group->GetBindingAsBufferBinding(bindingIndex).buffer; usageTracker->BufferUsedAs(buffer, wgpu::BufferUsage::Storage); } break; case wgpu::BindingType::SampledTexture: { - TextureBase* texture = group->GetBindingAsTextureView(i)->GetTexture(); + TextureBase* texture = + group->GetBindingAsTextureView(bindingIndex)->GetTexture(); usageTracker->TextureUsedAs(texture, wgpu::TextureUsage::Sampled); } break; case wgpu::BindingType::ReadonlyStorageBuffer: { - BufferBase* buffer = group->GetBindingAsBufferBinding(i).buffer; + BufferBase* buffer = group->GetBindingAsBufferBinding(bindingIndex).buffer; usageTracker->BufferUsedAs(buffer, kReadOnlyStorage); } break; @@ -131,7 +133,21 @@ namespace dawn_native { return DAWN_VALIDATION_ERROR("dynamicOffset count mismatch"); } - for (uint32_t i = 0; i < dynamicOffsetCount; ++i) { + const BindGroupLayoutBase::LayoutBindingInfo& layoutInfo = layout->GetBindingInfo(); + for (BindingIndex i = 0; i < dynamicOffsetCount; ++i) { + // BGL creation sorts bindings such that the dynamic buffer bindings are first. + // ASSERT that this true. + ASSERT(layoutInfo.hasDynamicOffset[i]); + switch (layoutInfo.types[i]) { + case wgpu::BindingType::UniformBuffer: + case wgpu::BindingType::StorageBuffer: + case wgpu::BindingType::ReadonlyStorageBuffer: + break; + default: + UNREACHABLE(); + break; + } + if (dynamicOffsets[i] % kMinDynamicBufferOffsetAlignment != 0) { return DAWN_VALIDATION_ERROR("Dynamic Buffer Offset need to be aligned"); } @@ -140,9 +156,9 @@ namespace dawn_native { // During BindGroup creation, validation ensures binding offset + binding size // <= buffer size. - DAWN_ASSERT(bufferBinding.buffer->GetSize() >= bufferBinding.size); - DAWN_ASSERT(bufferBinding.buffer->GetSize() - bufferBinding.size >= - bufferBinding.offset); + ASSERT(bufferBinding.buffer->GetSize() >= bufferBinding.size); + ASSERT(bufferBinding.buffer->GetSize() - bufferBinding.size >= + bufferBinding.offset); if ((dynamicOffsets[i] > bufferBinding.buffer->GetSize() - bufferBinding.offset - bufferBinding.size)) { diff --git a/src/dawn_native/ShaderModule.cpp b/src/dawn_native/ShaderModule.cpp index fc374ecd4d..503db34a26 100644 --- a/src/dawn_native/ShaderModule.cpp +++ b/src/dawn_native/ShaderModule.cpp @@ -378,13 +378,17 @@ namespace dawn_native { auto ExtractResourcesBinding = [this](std::vector bindings) -> MaybeError { for (const auto& binding : bindings) { - if (binding.binding >= kMaxBindingsPerGroup || binding.set >= kMaxBindGroups) { - return DAWN_VALIDATION_ERROR("Binding over limits in the SPIRV"); + if (binding.set >= kMaxBindGroups) { + return DAWN_VALIDATION_ERROR("Bind group index over limits in the SPIRV"); } - BindingInfo* info = &mBindingInfo[binding.set][binding.binding]; - *info = {}; - info->used = true; + const auto& it = mBindingInfo[binding.set].emplace(BindingNumber(binding.binding), + BindingInfo{}); + if (!it.second) { + return DAWN_VALIDATION_ERROR("Shader has duplicate bindings"); + } + + BindingInfo* info = &it.first->second; info->id = binding.id; info->base_type_id = binding.base_type_id; if (binding.binding_type == shaderc_spvc_binding_type_sampled_texture) { @@ -545,16 +549,20 @@ namespace dawn_native { return DAWN_VALIDATION_ERROR("No Descriptor Decoration set for resource"); } - uint32_t binding = compiler.get_decoration(resource.id, spv::DecorationBinding); + BindingNumber bindingNumber( + compiler.get_decoration(resource.id, spv::DecorationBinding)); uint32_t set = compiler.get_decoration(resource.id, spv::DecorationDescriptorSet); - if (binding >= kMaxBindingsPerGroup || set >= kMaxBindGroups) { - return DAWN_VALIDATION_ERROR("Binding over limits in the SPIRV"); + if (set >= kMaxBindGroups) { + return DAWN_VALIDATION_ERROR("Bind group index over limits in the SPIRV"); } - BindingInfo* info = &mBindingInfo[set][binding]; - *info = {}; - info->used = true; + const auto& it = mBindingInfo[set].emplace(bindingNumber, BindingInfo{}); + if (!it.second) { + return DAWN_VALIDATION_ERROR("Shader has duplicate bindings"); + } + + BindingInfo* info = &it.first->second; info->id = resource.id; info->base_type_id = resource.base_type_id; switch (bindingType) { @@ -716,10 +724,8 @@ namespace dawn_native { } for (uint32_t group : IterateBitSet(~layout->GetBindGroupLayoutsMask())) { - for (size_t i = 0; i < kMaxBindingsPerGroup; ++i) { - if (mBindingInfo[group][i].used) { - return false; - } + if (mBindingInfo[group].size() > 0) { + return false; } } @@ -731,14 +737,22 @@ namespace dawn_native { const BindGroupLayoutBase* layout) const { ASSERT(!IsError()); - const auto& layoutInfo = layout->GetBindingInfo(); - for (size_t i = 0; i < kMaxBindingsPerGroup; ++i) { - const auto& moduleInfo = mBindingInfo[group][i]; - const auto& layoutBindingType = layoutInfo.types[i]; + const BindGroupLayoutBase::LayoutBindingInfo& layoutInfo = layout->GetBindingInfo(); + const BindGroupLayoutBase::BindingMap& bindingMap = layout->GetBindingMap(); - if (!moduleInfo.used) { - continue; + // Iterate over all bindings used by this group in the shader, and find the + // corresponding binding in the BindGroupLayout, if it exists. + for (const auto& it : mBindingInfo[group]) { + BindingNumber bindingNumber = it.first; + const auto& moduleInfo = it.second; + + const auto& bindingIt = bindingMap.find(bindingNumber); + if (bindingIt == bindingMap.end()) { + return false; } + BindingIndex bindingIndex(bindingIt->second); + + const auto& layoutBindingType = layoutInfo.types[bindingIndex]; if (layoutBindingType != moduleInfo.type) { // Binding mismatch between shader and bind group is invalid. For example, a @@ -753,18 +767,17 @@ namespace dawn_native { } } - if ((layoutInfo.visibilities[i] & StageBit(mExecutionModel)) == 0) { + if ((layoutInfo.visibilities[bindingIndex] & StageBit(mExecutionModel)) == 0) { return false; } if (layoutBindingType == wgpu::BindingType::SampledTexture) { - Format::Type layoutTextureComponentType = - Format::TextureComponentTypeToFormatType(layoutInfo.textureComponentTypes[i]); + Format::Type layoutTextureComponentType = Format::TextureComponentTypeToFormatType( + layoutInfo.textureComponentTypes[bindingIndex]); if (layoutTextureComponentType != moduleInfo.textureComponentType) { return false; } - - if (layoutInfo.textureDimensions[i] != moduleInfo.textureDimension) { + if (layoutInfo.textureDimensions[bindingIndex] != moduleInfo.textureDimension) { return false; } } diff --git a/src/dawn_native/ShaderModule.h b/src/dawn_native/ShaderModule.h index 20f67a17e4..1797155303 100644 --- a/src/dawn_native/ShaderModule.h +++ b/src/dawn_native/ShaderModule.h @@ -20,6 +20,7 @@ #include "dawn_native/Error.h" #include "dawn_native/Format.h" #include "dawn_native/Forward.h" +#include "dawn_native/IntegerTypes.h" #include "dawn_native/PerStage.h" #include "dawn_native/dawn_platform.h" @@ -28,6 +29,7 @@ #include #include +#include #include namespace spirv_cross { @@ -57,11 +59,9 @@ namespace dawn_native { wgpu::TextureViewDimension textureDimension = wgpu::TextureViewDimension::Undefined; Format::Type textureComponentType = Format::Type::Float; bool multisampled = false; - bool used = false; wgpu::TextureFormat storageTextureFormat = wgpu::TextureFormat::Undefined; }; - using ModuleBindingInfo = - std::array, kMaxBindGroups>; + using ModuleBindingInfo = std::array, kMaxBindGroups>; const ModuleBindingInfo& GetBindingInfo() const; const std::bitset& GetUsedVertexAttributes() const; diff --git a/src/dawn_native/d3d12/BindGroupD3D12.cpp b/src/dawn_native/d3d12/BindGroupD3D12.cpp index 6a708133c2..6ef4bc25c0 100644 --- a/src/dawn_native/d3d12/BindGroupD3D12.cpp +++ b/src/dawn_native/d3d12/BindGroupD3D12.cpp @@ -81,13 +81,13 @@ namespace dawn_native { namespace d3d12 { mLastUsageSerial = pendingSerial; mHeapSerial = allocator->GetShaderVisibleHeapsSerial(); - const auto& layout = bgl->GetBindingInfo(); + const BindGroupLayoutBase::LayoutBindingInfo& layout = bgl->GetBindingInfo(); const auto& bindingOffsets = bgl->GetBindingOffsets(); ID3D12Device* d3d12Device = device->GetD3D12Device().Get(); - for (uint32_t bindingIndex : IterateBitSet(layout.mask)) { + for (BindingIndex bindingIndex = 0; bindingIndex < layout.bindingCount; ++bindingIndex) { // It's not necessary to create descriptors in descriptor heap for dynamic // resources. So skip allocating descriptors in descriptor heaps for dynamic // buffers. diff --git a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp b/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp index 0ed46415c5..50a4b03f3e 100644 --- a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp +++ b/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp @@ -45,19 +45,19 @@ namespace dawn_native { namespace d3d12 { : BindGroupLayoutBase(device, descriptor), mDescriptorCounts{}, mBindGroupAllocator(MakeFrontendBindGroupAllocator(4096)) { - const auto& groupInfo = GetBindingInfo(); + const BindGroupLayoutBase::LayoutBindingInfo& groupInfo = GetBindingInfo(); - for (uint32_t binding : IterateBitSet(groupInfo.mask)) { + for (BindingIndex bindingIndex = 0; bindingIndex < groupInfo.bindingCount; ++bindingIndex) { // For dynamic resources, Dawn uses root descriptor in D3D12 backend. // So there is no need to allocate the descriptor from descriptor heap. Skip counting // dynamic resources for calculating size of descriptor heap. - if (groupInfo.hasDynamicOffset[binding]) { + if (groupInfo.hasDynamicOffset[bindingIndex]) { continue; } DescriptorType descriptorType = - WGPUBindingTypeToDescriptorType(groupInfo.types[binding]); - mBindingOffsets[binding] = mDescriptorCounts[descriptorType]++; + WGPUBindingTypeToDescriptorType(groupInfo.types[bindingIndex]); + mBindingOffsets[bindingIndex] = mDescriptorCounts[descriptorType]++; } auto SetDescriptorRange = [&](uint32_t index, uint32_t count, uint32_t* baseRegister, @@ -101,16 +101,16 @@ namespace dawn_native { namespace d3d12 { D3D12_DESCRIPTOR_RANGE_TYPE_SAMPLER); descriptorOffsets[Sampler] = 0; - for (uint32_t binding : IterateBitSet(groupInfo.mask)) { - if (groupInfo.hasDynamicOffset[binding]) { + for (BindingIndex bindingIndex = 0; bindingIndex < groupInfo.bindingCount; ++bindingIndex) { + if (groupInfo.hasDynamicOffset[bindingIndex]) { // Dawn is using values in mBindingOffsets to decide register number in HLSL. // Root descriptor needs to set this value to set correct register number in // generated HLSL shader. - switch (groupInfo.types[binding]) { + switch (groupInfo.types[bindingIndex]) { case wgpu::BindingType::UniformBuffer: case wgpu::BindingType::StorageBuffer: case wgpu::BindingType::ReadonlyStorageBuffer: - mBindingOffsets[binding] = baseRegister++; + mBindingOffsets[bindingIndex] = baseRegister++; break; case wgpu::BindingType::SampledTexture: case wgpu::BindingType::Sampler: @@ -125,8 +125,8 @@ namespace dawn_native { namespace d3d12 { // TODO(shaobo.yan@intel.com): Implement dynamic buffer offset. DescriptorType descriptorType = - WGPUBindingTypeToDescriptorType(groupInfo.types[binding]); - mBindingOffsets[binding] += descriptorOffsets[descriptorType]; + WGPUBindingTypeToDescriptorType(groupInfo.types[bindingIndex]); + mBindingOffsets[bindingIndex] += descriptorOffsets[descriptorType]; } } diff --git a/src/dawn_native/d3d12/ShaderModuleD3D12.cpp b/src/dawn_native/d3d12/ShaderModuleD3D12.cpp index 6a6075da86..758af39950 100644 --- a/src/dawn_native/d3d12/ShaderModuleD3D12.cpp +++ b/src/dawn_native/d3d12/ShaderModuleD3D12.cpp @@ -93,23 +93,23 @@ namespace dawn_native { namespace d3d12 { const ModuleBindingInfo& moduleBindingInfo = GetBindingInfo(); for (uint32_t group : IterateBitSet(layout->GetBindGroupLayoutsMask())) { - const auto& bindingOffsets = - ToBackend(layout->GetBindGroupLayout(group))->GetBindingOffsets(); + const BindGroupLayout* bgl = ToBackend(layout->GetBindGroupLayout(group)); + const auto& bindingOffsets = bgl->GetBindingOffsets(); const auto& groupBindingInfo = moduleBindingInfo[group]; - for (uint32_t binding = 0; binding < groupBindingInfo.size(); ++binding) { - const BindingInfo& bindingInfo = groupBindingInfo[binding]; - if (bindingInfo.used) { - uint32_t bindingOffset = bindingOffsets[binding]; - if (GetDevice()->IsToggleEnabled(Toggle::UseSpvc)) { - DAWN_TRY(CheckSpvcSuccess( - mSpvcContext.SetDecoration( - bindingInfo.id, SHADERC_SPVC_DECORATION_BINDING, bindingOffset), - "Unable to set decorating binding before generating HLSL shader w/ " - "spvc")); - } else { - compiler->set_decoration(bindingInfo.id, spv::DecorationBinding, - bindingOffset); - } + for (const auto& it : groupBindingInfo) { + const BindingInfo& bindingInfo = it.second; + BindingNumber bindingNumber = it.first; + BindingIndex bindingIndex = bgl->GetBindingIndex(bindingNumber); + + uint32_t bindingOffset = bindingOffsets[bindingIndex]; + if (GetDevice()->IsToggleEnabled(Toggle::UseSpvc)) { + DAWN_TRY(CheckSpvcSuccess( + mSpvcContext.SetDecoration(bindingInfo.id, SHADERC_SPVC_DECORATION_BINDING, + bindingOffset), + "Unable to set decorating binding before generating HLSL shader w/ " + "spvc")); + } else { + compiler->set_decoration(bindingInfo.id, spv::DecorationBinding, bindingOffset); } } } diff --git a/src/dawn_native/metal/CommandBufferMTL.mm b/src/dawn_native/metal/CommandBufferMTL.mm index 8d85724bce..5d325c721e 100644 --- a/src/dawn_native/metal/CommandBufferMTL.mm +++ b/src/dawn_native/metal/CommandBufferMTL.mm @@ -493,13 +493,15 @@ namespace dawn_native { namespace metal { uint32_t dynamicOffsetCount, uint64_t* dynamicOffsets, PipelineLayout* pipelineLayout) { - const auto& layout = group->GetLayout()->GetBindingInfo(); + const BindGroupLayoutBase::LayoutBindingInfo& layout = + group->GetLayout()->GetBindingInfo(); uint32_t currentDynamicBufferIndex = 0; // TODO(kainino@chromium.org): Maintain buffers and offsets arrays in BindGroup // so that we only have to do one setVertexBuffers and one setFragmentBuffers // call here. - for (uint32_t bindingIndex : IterateBitSet(layout.mask)) { + for (BindingIndex bindingIndex = 0; bindingIndex < layout.bindingCount; + ++bindingIndex) { auto stage = layout.visibilities[bindingIndex]; bool hasVertStage = stage & wgpu::ShaderStage::Vertex && render != nil; bool hasFragStage = stage & wgpu::ShaderStage::Fragment && render != nil; diff --git a/src/dawn_native/metal/PipelineLayoutMTL.mm b/src/dawn_native/metal/PipelineLayoutMTL.mm index 3a092380fa..4e41ef6a27 100644 --- a/src/dawn_native/metal/PipelineLayoutMTL.mm +++ b/src/dawn_native/metal/PipelineLayoutMTL.mm @@ -29,28 +29,27 @@ namespace dawn_native { namespace metal { uint32_t textureIndex = 0; for (uint32_t group : IterateBitSet(GetBindGroupLayoutsMask())) { - const auto& groupInfo = GetBindGroupLayout(group)->GetBindingInfo(); - for (size_t binding = 0; binding < kMaxBindingsPerGroup; ++binding) { - if (!(groupInfo.visibilities[binding] & StageBit(stage))) { - continue; - } - if (!groupInfo.mask[binding]) { + const BindGroupLayoutBase::LayoutBindingInfo& groupInfo = + GetBindGroupLayout(group)->GetBindingInfo(); + for (BindingIndex bindingIndex = 0; bindingIndex < groupInfo.bindingCount; + ++bindingIndex) { + if (!(groupInfo.visibilities[bindingIndex] & StageBit(stage))) { continue; } - switch (groupInfo.types[binding]) { + switch (groupInfo.types[bindingIndex]) { case wgpu::BindingType::UniformBuffer: case wgpu::BindingType::StorageBuffer: case wgpu::BindingType::ReadonlyStorageBuffer: - mIndexInfo[stage][group][binding] = bufferIndex; + mIndexInfo[stage][group][bindingIndex] = bufferIndex; bufferIndex++; break; case wgpu::BindingType::Sampler: - mIndexInfo[stage][group][binding] = samplerIndex; + mIndexInfo[stage][group][bindingIndex] = samplerIndex; samplerIndex++; break; case wgpu::BindingType::SampledTexture: - mIndexInfo[stage][group][binding] = textureIndex; + mIndexInfo[stage][group][bindingIndex] = textureIndex; textureIndex++; break; case wgpu::BindingType::StorageTexture: diff --git a/src/dawn_native/metal/ShaderModuleMTL.mm b/src/dawn_native/metal/ShaderModuleMTL.mm index 5028934c68..15f4cc2c23 100644 --- a/src/dawn_native/metal/ShaderModuleMTL.mm +++ b/src/dawn_native/metal/ShaderModuleMTL.mm @@ -131,17 +131,24 @@ namespace dawn_native { namespace metal { // Create one resource binding entry per stage per binding. for (uint32_t group : IterateBitSet(layout->GetBindGroupLayoutsMask())) { - const auto& bgInfo = layout->GetBindGroupLayout(group)->GetBindingInfo(); - for (uint32_t binding : IterateBitSet(bgInfo.mask)) { - for (auto stage : IterateStages(bgInfo.visibilities[binding])) { - uint32_t index = layout->GetBindingIndexInfo(stage)[group][binding]; + const BindGroupLayoutBase::LayoutBindingInfo& bgInfo = + layout->GetBindGroupLayout(group)->GetBindingInfo(); + const BindGroupLayoutBase::BindingMap& bindingMap = + layout->GetBindGroupLayout(group)->GetBindingMap(); + + for (const auto& it : bindingMap) { + uint32_t binding = it.first; + BindingIndex bindingIndex = it.second; + + for (auto stage : IterateStages(bgInfo.visibilities[bindingIndex])) { + uint32_t shaderIndex = layout->GetBindingIndexInfo(stage)[group][bindingIndex]; if (GetDevice()->IsToggleEnabled(Toggle::UseSpvc)) { shaderc_spvc_msl_resource_binding mslBinding; mslBinding.stage = ToSpvcExecutionModel(stage); mslBinding.desc_set = group; mslBinding.binding = binding; mslBinding.msl_buffer = mslBinding.msl_texture = mslBinding.msl_sampler = - index; + shaderIndex; DAWN_TRY(CheckSpvcSuccess(mSpvcContext.AddMSLResourceBinding(mslBinding), "Unable to add MSL Resource Binding")); } else { @@ -150,7 +157,7 @@ namespace dawn_native { namespace metal { mslBinding.desc_set = group; mslBinding.binding = binding; mslBinding.msl_buffer = mslBinding.msl_texture = mslBinding.msl_sampler = - index; + shaderIndex; compiler->add_msl_resource_binding(mslBinding); } diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index 25d3c81265..f77216256d 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -239,10 +239,12 @@ namespace dawn_native { namespace opengl { uint32_t dynamicOffsetCount, uint64_t* dynamicOffsets) { const auto& indices = ToBackend(mPipelineLayout)->GetBindingIndexInfo()[index]; - const auto& layout = group->GetLayout()->GetBindingInfo(); + const BindGroupLayoutBase::LayoutBindingInfo& layout = + group->GetLayout()->GetBindingInfo(); uint32_t currentDynamicIndex = 0; - for (uint32_t bindingIndex : IterateBitSet(layout.mask)) { + for (BindingIndex bindingIndex = 0; bindingIndex < layout.bindingCount; + ++bindingIndex) { switch (layout.types[bindingIndex]) { case wgpu::BindingType::UniformBuffer: { BufferBinding binding = group->GetBindingAsBufferBinding(bindingIndex); diff --git a/src/dawn_native/opengl/PipelineGL.cpp b/src/dawn_native/opengl/PipelineGL.cpp index 71e2b83fb9..eeda875209 100644 --- a/src/dawn_native/opengl/PipelineGL.cpp +++ b/src/dawn_native/opengl/PipelineGL.cpp @@ -107,19 +107,20 @@ namespace dawn_native { namespace opengl { const auto& indices = layout->GetBindingIndexInfo(); for (uint32_t group : IterateBitSet(layout->GetBindGroupLayoutsMask())) { - const auto& groupInfo = layout->GetBindGroupLayout(group)->GetBindingInfo(); + const BindGroupLayoutBase::LayoutBindingInfo& groupInfo = + layout->GetBindGroupLayout(group)->GetBindingInfo(); - for (uint32_t binding = 0; binding < kMaxBindingsPerGroup; ++binding) { - if (!groupInfo.mask[binding]) { - continue; - } + for (const auto& it : layout->GetBindGroupLayout(group)->GetBindingMap()) { + BindingNumber bindingNumber = it.first; + BindingIndex bindingIndex = it.second; - std::string name = GetBindingName(group, binding); - switch (groupInfo.types[binding]) { + std::string name = GetBindingName(group, bindingNumber); + switch (groupInfo.types[bindingIndex]) { case wgpu::BindingType::UniformBuffer: { GLint location = gl.GetUniformBlockIndex(mProgram, name.c_str()); if (location != -1) { - gl.UniformBlockBinding(mProgram, location, indices[group][binding]); + gl.UniformBlockBinding(mProgram, location, + indices[group][bindingIndex]); } } break; @@ -129,7 +130,7 @@ namespace dawn_native { namespace opengl { mProgram, GL_SHADER_STORAGE_BLOCK, name.c_str()); if (location != GL_INVALID_INDEX) { gl.ShaderStorageBlockBinding(mProgram, location, - indices[group][binding]); + indices[group][bindingIndex]); } } break; diff --git a/src/dawn_native/opengl/PipelineLayoutGL.cpp b/src/dawn_native/opengl/PipelineLayoutGL.cpp index 18fd4f2fe1..7048d5ec05 100644 --- a/src/dawn_native/opengl/PipelineLayoutGL.cpp +++ b/src/dawn_native/opengl/PipelineLayoutGL.cpp @@ -28,30 +28,28 @@ namespace dawn_native { namespace opengl { GLuint ssboIndex = 0; for (uint32_t group : IterateBitSet(GetBindGroupLayoutsMask())) { - const auto& groupInfo = GetBindGroupLayout(group)->GetBindingInfo(); + const BindGroupLayoutBase::LayoutBindingInfo& groupInfo = + GetBindGroupLayout(group)->GetBindingInfo(); - for (size_t binding = 0; binding < kMaxBindingsPerGroup; ++binding) { - if (!groupInfo.mask[binding]) { - continue; - } - - switch (groupInfo.types[binding]) { + for (BindingIndex bindingIndex = 0; bindingIndex < groupInfo.bindingCount; + ++bindingIndex) { + switch (groupInfo.types[bindingIndex]) { case wgpu::BindingType::UniformBuffer: - mIndexInfo[group][binding] = uboIndex; + mIndexInfo[group][bindingIndex] = uboIndex; uboIndex++; break; case wgpu::BindingType::Sampler: - mIndexInfo[group][binding] = samplerIndex; + mIndexInfo[group][bindingIndex] = samplerIndex; samplerIndex++; break; case wgpu::BindingType::SampledTexture: - mIndexInfo[group][binding] = sampledTextureIndex; + mIndexInfo[group][bindingIndex] = sampledTextureIndex; sampledTextureIndex++; break; case wgpu::BindingType::StorageBuffer: case wgpu::BindingType::ReadonlyStorageBuffer: - mIndexInfo[group][binding] = ssboIndex; + mIndexInfo[group][bindingIndex] = ssboIndex; ssboIndex++; break; diff --git a/src/dawn_native/opengl/ShaderModuleGL.cpp b/src/dawn_native/opengl/ShaderModuleGL.cpp index 67e6ebf38c..4174fe413a 100644 --- a/src/dawn_native/opengl/ShaderModuleGL.cpp +++ b/src/dawn_native/opengl/ShaderModuleGL.cpp @@ -123,7 +123,7 @@ namespace dawn_native { namespace opengl { DAWN_TRY(ExtractSpirvInfo(*compiler)); - const auto& bindingInfo = GetBindingInfo(); + const ShaderModuleBase::ModuleBindingInfo& bindingInfo = GetBindingInfo(); // Extract bindings names so that it can be used to get its location in program. // Now translate the separate sampler / textures into combined ones and store their info. @@ -173,19 +173,18 @@ namespace dawn_native { namespace opengl { // Also unsets the SPIRV "Binding" decoration as it outputs "layout(binding=)" which // isn't supported on OSX's OpenGL. for (uint32_t group = 0; group < kMaxBindGroups; ++group) { - for (uint32_t binding = 0; binding < kMaxBindingsPerGroup; ++binding) { - const auto& info = bindingInfo[group][binding]; - if (info.used) { - if (GetDevice()->IsToggleEnabled(Toggle::UseSpvc)) { - mSpvcContext.SetName(info.base_type_id, GetBindingName(group, binding)); - mSpvcContext.UnsetDecoration(info.id, shaderc_spvc_decoration_binding); - mSpvcContext.UnsetDecoration(info.id, - shaderc_spvc_decoration_descriptorset); - } else { - compiler->set_name(info.base_type_id, GetBindingName(group, binding)); - compiler->unset_decoration(info.id, spv::DecorationBinding); - compiler->unset_decoration(info.id, spv::DecorationDescriptorSet); - } + for (const auto& it : bindingInfo[group]) { + BindingNumber bindingNumber = it.first; + const auto& info = it.second; + + if (GetDevice()->IsToggleEnabled(Toggle::UseSpvc)) { + mSpvcContext.SetName(info.base_type_id, GetBindingName(group, bindingNumber)); + mSpvcContext.UnsetDecoration(info.id, shaderc_spvc_decoration_binding); + mSpvcContext.UnsetDecoration(info.id, shaderc_spvc_decoration_descriptorset); + } else { + compiler->set_name(info.base_type_id, GetBindingName(group, bindingNumber)); + compiler->unset_decoration(info.id, spv::DecorationBinding); + compiler->unset_decoration(info.id, spv::DecorationDescriptorSet); } } } diff --git a/src/dawn_native/vulkan/BindGroupLayoutVk.cpp b/src/dawn_native/vulkan/BindGroupLayoutVk.cpp index 0d2491b4ec..d26eebbb8e 100644 --- a/src/dawn_native/vulkan/BindGroupLayoutVk.cpp +++ b/src/dawn_native/vulkan/BindGroupLayoutVk.cpp @@ -89,14 +89,17 @@ namespace dawn_native { namespace vulkan { // bindings of the same type. uint32_t numBindings = 0; std::array bindings; - for (uint32_t bindingIndex : IterateBitSet(info.mask)) { - VkDescriptorSetLayoutBinding* binding = &bindings[numBindings]; - binding->binding = bindingIndex; - binding->descriptorType = + for (const auto& it : GetBindingMap()) { + BindingNumber bindingNumber = it.first; + BindingIndex bindingIndex = it.second; + + VkDescriptorSetLayoutBinding* vkBinding = &bindings[numBindings]; + vkBinding->binding = bindingNumber; + vkBinding->descriptorType = VulkanDescriptorType(info.types[bindingIndex], info.hasDynamicOffset[bindingIndex]); - binding->descriptorCount = 1; - binding->stageFlags = VulkanShaderStageFlags(info.visibilities[bindingIndex]); - binding->pImmutableSamplers = nullptr; + vkBinding->descriptorCount = 1; + vkBinding->stageFlags = VulkanShaderStageFlags(info.visibilities[bindingIndex]); + vkBinding->pImmutableSamplers = nullptr; numBindings++; } @@ -116,7 +119,7 @@ namespace dawn_native { namespace vulkan { // Compute the size of descriptor pools used for this layout. std::map descriptorCountPerType; - for (uint32_t bindingIndex : IterateBitSet(info.mask)) { + for (BindingIndex bindingIndex = 0; bindingIndex < info.bindingCount; ++bindingIndex) { VkDescriptorType vulkanType = VulkanDescriptorType(info.types[bindingIndex], info.hasDynamicOffset[bindingIndex]); diff --git a/src/dawn_native/vulkan/BindGroupVk.cpp b/src/dawn_native/vulkan/BindGroupVk.cpp index 164c53795a..fb6aa3796b 100644 --- a/src/dawn_native/vulkan/BindGroupVk.cpp +++ b/src/dawn_native/vulkan/BindGroupVk.cpp @@ -43,13 +43,16 @@ namespace dawn_native { namespace vulkan { std::array writeBufferInfo; std::array writeImageInfo; - const auto& layoutInfo = GetLayout()->GetBindingInfo(); - for (uint32_t bindingIndex : IterateBitSet(layoutInfo.mask)) { + const BindGroupLayoutBase::LayoutBindingInfo& layoutInfo = GetLayout()->GetBindingInfo(); + for (const auto& it : GetLayout()->GetBindingMap()) { + BindingNumber bindingNumber = it.first; + BindingIndex bindingIndex = it.second; + auto& write = writes[numWrites]; write.sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET; write.pNext = nullptr; write.dstSet = GetHandle(); - write.dstBinding = bindingIndex; + write.dstBinding = bindingNumber; write.dstArrayElement = 0; write.descriptorCount = 1; write.descriptorType = VulkanDescriptorType(layoutInfo.types[bindingIndex], diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp index ab70419cc9..9f892e847e 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.cpp +++ b/src/dawn_native/vulkan/CommandBufferVk.cpp @@ -140,10 +140,10 @@ namespace dawn_native { namespace vulkan { mDynamicOffsetCounts, mDynamicOffsets); for (uint32_t index : IterateBitSet(mBindGroupLayoutsMask)) { - for (uint32_t binding : IterateBitSet(mBuffersNeedingBarrier[index])) { - switch (mBindingTypes[index][binding]) { + for (uint32_t bindingIndex : IterateBitSet(mBuffersNeedingBarrier[index])) { + switch (mBindingTypes[index][bindingIndex]) { case wgpu::BindingType::StorageBuffer: - ToBackend(mBuffers[index][binding]) + ToBackend(mBuffers[index][bindingIndex]) ->TransitionUsageNow(recordingContext, wgpu::BufferUsage::Storage); break; diff --git a/src/tests/end2end/BindGroupTests.cpp b/src/tests/end2end/BindGroupTests.cpp index 687645a950..daa6151e8a 100644 --- a/src/tests/end2end/BindGroupTests.cpp +++ b/src/tests/end2end/BindGroupTests.cpp @@ -780,4 +780,101 @@ TEST_P(BindGroupTests, BindGroupLayoutVisibilityCanBeNone) { queue.Submit(1, &commands); } +// Test that bind group bindings may have unbounded and arbitrary binding numbers +TEST_P(BindGroupTests, ArbitraryBindingNumbers) { + utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, kRTSize, kRTSize); + + wgpu::ShaderModule vsModule = + utils::CreateShaderModule(device, utils::SingleShaderStage::Vertex, R"( + #version 450 + void main() { + const vec2 pos[3] = vec2[3](vec2(-1.f, 1.f), vec2(1.f, 1.f), vec2(-1.f, -1.f)); + gl_Position = vec4(pos[gl_VertexIndex], 0.f, 1.f); + })"); + + wgpu::ShaderModule fsModule = + utils::CreateShaderModule(device, utils::SingleShaderStage::Fragment, R"( + #version 450 + layout (set = 0, binding = 953) uniform ubo1 { + vec4 color1; + }; + layout (set = 0, binding = 47) uniform ubo2 { + vec4 color2; + }; + layout (set = 0, binding = 111) uniform ubo3 { + vec4 color3; + }; + layout(location = 0) out vec4 fragColor; + void main() { + fragColor = color1 + 2 * color2 + 4 * color3; + })"); + + utils::ComboRenderPipelineDescriptor pipelineDescriptor(device); + pipelineDescriptor.vertexStage.module = vsModule; + pipelineDescriptor.cFragmentStage.module = fsModule; + pipelineDescriptor.cColorStates[0].format = renderPass.colorFormat; + + wgpu::RenderPipeline pipeline = device.CreateRenderPipeline(&pipelineDescriptor); + + wgpu::Buffer black = + utils::CreateBufferFromData(device, wgpu::BufferUsage::Uniform, {0.f, 0.f, 0.f, 0.f}); + wgpu::Buffer red = + utils::CreateBufferFromData(device, wgpu::BufferUsage::Uniform, {0.251f, 0.0f, 0.0f, 0.0f}); + wgpu::Buffer green = + utils::CreateBufferFromData(device, wgpu::BufferUsage::Uniform, {0.0f, 0.251f, 0.0f, 0.0f}); + wgpu::Buffer blue = + utils::CreateBufferFromData(device, wgpu::BufferUsage::Uniform, {0.0f, 0.0f, 0.251f, 0.0f}); + + auto DoTest = [&](wgpu::Buffer color1, wgpu::Buffer color2, wgpu::Buffer color3, RGBA8 filled) { + auto DoTestInner = [&](wgpu::BindGroup bindGroup) { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass.renderPassInfo); + pass.SetPipeline(pipeline); + pass.SetBindGroup(0, bindGroup); + pass.Draw(3, 1, 0, 0); + pass.EndPass(); + + wgpu::CommandBuffer commands = encoder.Finish(); + queue.Submit(1, &commands); + + EXPECT_PIXEL_RGBA8_EQ(filled, renderPass.color, 1, 1); + }; + + utils::BindingInitializationHelper bindings[] = { + {953, color1, 0, 4 * sizeof(float)}, // + {47, color2, 0, 4 * sizeof(float)}, // + {111, color3, 0, 4 * sizeof(float)}, // + }; + + // Should work regardless of what order the bindings are specified in. + DoTestInner(utils::MakeBindGroup(device, pipeline.GetBindGroupLayout(0), + {bindings[0], bindings[1], bindings[2]})); + DoTestInner(utils::MakeBindGroup(device, pipeline.GetBindGroupLayout(0), + {bindings[1], bindings[0], bindings[2]})); + DoTestInner(utils::MakeBindGroup(device, pipeline.GetBindGroupLayout(0), + {bindings[2], bindings[0], bindings[1]})); + }; + + // first color is normal, second is 2x, third is 3x. + DoTest(black, black, black, RGBA8(0, 0, 0, 0)); + + // Check the first binding maps to the first slot. We know this because the colors are + // multiplied 1x. + DoTest(red, black, black, RGBA8(64, 0, 0, 0)); + DoTest(green, black, black, RGBA8(0, 64, 0, 0)); + DoTest(blue, black, black, RGBA8(0, 0, 64, 0)); + + // Use multiple bindings and check the second color maps to the second slot. + // We know this because the second slot is multiplied 2x. + DoTest(green, blue, black, RGBA8(0, 64, 128, 0)); + DoTest(blue, green, black, RGBA8(0, 128, 64, 0)); + DoTest(red, green, black, RGBA8(64, 128, 0, 0)); + + // Use multiple bindings and check the third color maps to the third slot. + // We know this because the third slot is multiplied 4x. + DoTest(black, blue, red, RGBA8(255, 0, 128, 0)); + DoTest(blue, black, green, RGBA8(0, 255, 64, 0)); + DoTest(red, black, blue, RGBA8(64, 0, 255, 0)); +} + DAWN_INSTANTIATE_TEST(BindGroupTests, D3D12Backend(), MetalBackend(), OpenGLBackend(), VulkanBackend()); diff --git a/src/tests/unittests/validation/BindGroupValidationTests.cpp b/src/tests/unittests/validation/BindGroupValidationTests.cpp index 0b11b0bfce..b7f0120fb9 100644 --- a/src/tests/unittests/validation/BindGroupValidationTests.cpp +++ b/src/tests/unittests/validation/BindGroupValidationTests.cpp @@ -510,16 +510,15 @@ TEST_F(BindGroupLayoutValidationTest, BindGroupLayoutStorageBindingsInVertexShad device, {{0, wgpu::ShaderStage::Fragment, wgpu::BindingType::ReadonlyStorageBuffer}}); } -// Tests setting OOB checks for kMaxBindingsPerGroup in bind group layouts. -TEST_F(BindGroupLayoutValidationTest, BindGroupLayoutBindingOOB) { - // Checks that kMaxBindingsPerGroup - 1 is valid. - utils::MakeBindGroupLayout(device, {{kMaxBindingsPerGroup - 1, wgpu::ShaderStage::Vertex, +// Tests setting that bind group layout bindings numbers may be >= kMaxBindingsPerGroup. +TEST_F(BindGroupLayoutValidationTest, BindGroupLayoutBindingUnbounded) { + // Checks that kMaxBindingsPerGroup is valid. + utils::MakeBindGroupLayout(device, {{kMaxBindingsPerGroup, wgpu::ShaderStage::Vertex, wgpu::BindingType::UniformBuffer}}); - // Checks that kMaxBindingsPerGroup is OOB - ASSERT_DEVICE_ERROR(utils::MakeBindGroupLayout( - device, - {{kMaxBindingsPerGroup, wgpu::ShaderStage::Vertex, wgpu::BindingType::UniformBuffer}})); + // Checks that kMaxBindingsPerGroup + 1 is valid. + utils::MakeBindGroupLayout(device, {{kMaxBindingsPerGroup + 1, wgpu::ShaderStage::Vertex, + wgpu::BindingType::UniformBuffer}}); } // This test verifies that the BindGroupLayout bindings are correctly validated, even if the @@ -698,8 +697,10 @@ class SetBindGroupValidationTest : public ValidationTest { device, {{0, wgpu::ShaderStage::Compute | wgpu::ShaderStage::Fragment, wgpu::BindingType::UniformBuffer, true}, {1, wgpu::ShaderStage::Compute | wgpu::ShaderStage::Fragment, - wgpu::BindingType::StorageBuffer, true}, + wgpu::BindingType::UniformBuffer, false}, {2, wgpu::ShaderStage::Compute | wgpu::ShaderStage::Fragment, + wgpu::BindingType::StorageBuffer, true}, + {3, wgpu::ShaderStage::Compute | wgpu::ShaderStage::Fragment, wgpu::BindingType::ReadonlyStorageBuffer, true}}); } @@ -723,13 +724,16 @@ class SetBindGroupValidationTest : public ValidationTest { wgpu::ShaderModule fsModule = utils::CreateShaderModule(device, utils::SingleShaderStage::Fragment, R"( #version 450 - layout(std140, set = 0, binding = 0) uniform uBuffer { + layout(std140, set = 0, binding = 0) uniform uBufferDynamic { + vec2 value0; + }; + layout(std140, set = 0, binding = 1) uniform uBuffer { vec2 value1; }; - layout(std140, set = 0, binding = 1) buffer SBuffer { + layout(std140, set = 0, binding = 2) buffer SBufferDynamic { vec2 value2; } sBuffer; - layout(std140, set = 0, binding = 2) readonly buffer RBuffer { + layout(std140, set = 0, binding = 3) readonly buffer RBufferDynamic { vec2 value3; } rBuffer; layout(location = 0) out vec4 fragColor; @@ -753,13 +757,16 @@ class SetBindGroupValidationTest : public ValidationTest { const uint kInstances = 11; layout(local_size_x = kTileSize, local_size_y = kTileSize, local_size_z = 1) in; - layout(std140, set = 0, binding = 0) uniform UniformBuffer { + layout(std140, set = 0, binding = 0) uniform UniformBufferDynamic { + float value0; + }; + layout(std140, set = 0, binding = 1) uniform UniformBuffer { float value1; }; - layout(std140, set = 0, binding = 1) buffer SBuffer { + layout(std140, set = 0, binding = 2) buffer SBufferDynamic { float value2; } dst; - layout(std140, set = 0, binding = 2) readonly buffer RBuffer { + layout(std140, set = 0, binding = 3) readonly buffer RBufferDynamic { readonly float value3; } rdst; void main() { @@ -824,8 +831,9 @@ TEST_F(SetBindGroupValidationTest, Basic) { wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage); wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout, {{0, uniformBuffer, 0, kBindingSize}, - {1, storageBuffer, 0, kBindingSize}, - {2, readonlyStorageBuffer, 0, kBindingSize}}); + {1, uniformBuffer, 0, kBindingSize}, + {2, storageBuffer, 0, kBindingSize}, + {3, readonlyStorageBuffer, 0, kBindingSize}}); std::array offsets = {512, 256, 0}; @@ -842,8 +850,9 @@ TEST_F(SetBindGroupValidationTest, DynamicOffsetsMismatch) { wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage); wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout, {{0, uniformBuffer, 0, kBindingSize}, - {1, storageBuffer, 0, kBindingSize}, - {2, readonlyStorageBuffer, 0, kBindingSize}}); + {1, uniformBuffer, 0, kBindingSize}, + {2, storageBuffer, 0, kBindingSize}, + {3, readonlyStorageBuffer, 0, kBindingSize}}); // Number of offsets mismatch. std::array mismatchOffsets = {768, 512, 256, 0}; @@ -865,8 +874,9 @@ TEST_F(SetBindGroupValidationTest, DynamicOffsetsNotAligned) { wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage); wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout, {{0, uniformBuffer, 0, kBindingSize}, - {1, storageBuffer, 0, kBindingSize}, - {2, readonlyStorageBuffer, 0, kBindingSize}}); + {1, uniformBuffer, 0, kBindingSize}, + {2, storageBuffer, 0, kBindingSize}, + {3, readonlyStorageBuffer, 0, kBindingSize}}); // Dynamic offsets are not aligned. std::array notAlignedOffsets = {512, 128, 0}; @@ -884,8 +894,9 @@ TEST_F(SetBindGroupValidationTest, OffsetOutOfBoundDynamicUniformBuffer) { wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage); wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout, {{0, uniformBuffer, 0, kBindingSize}, - {1, storageBuffer, 0, kBindingSize}, - {2, readonlyStorageBuffer, 0, kBindingSize}}); + {1, uniformBuffer, 0, kBindingSize}, + {2, storageBuffer, 0, kBindingSize}, + {3, readonlyStorageBuffer, 0, kBindingSize}}); // Dynamic offset + offset is larger than buffer size. std::array overFlowOffsets = {1024, 256, 0}; @@ -903,8 +914,9 @@ TEST_F(SetBindGroupValidationTest, OffsetOutOfBoundDynamicStorageBuffer) { wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage); wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout, {{0, uniformBuffer, 0, kBindingSize}, - {1, storageBuffer, 0, kBindingSize}, - {2, readonlyStorageBuffer, 0, kBindingSize}}); + {1, uniformBuffer, 0, kBindingSize}, + {2, storageBuffer, 0, kBindingSize}, + {3, readonlyStorageBuffer, 0, kBindingSize}}); // Dynamic offset + offset is larger than buffer size. std::array overFlowOffsets = {0, 256, 1024}; @@ -922,8 +934,9 @@ TEST_F(SetBindGroupValidationTest, BindingSizeOutOfBoundDynamicUniformBuffer) { wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage); wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout, {{0, uniformBuffer, 0, kBindingSize}, - {1, storageBuffer, 0, kBindingSize}, - {2, readonlyStorageBuffer, 0, kBindingSize}}); + {1, uniformBuffer, 0, kBindingSize}, + {2, storageBuffer, 0, kBindingSize}, + {3, readonlyStorageBuffer, 0, kBindingSize}}); // Dynamic offset + offset isn't larger than buffer size. // But with binding size, it will trigger OOB error. @@ -941,8 +954,9 @@ TEST_F(SetBindGroupValidationTest, BindingSizeOutOfBoundDynamicStorageBuffer) { wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage); wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout, {{0, uniformBuffer, 0, kBindingSize}, - {1, storageBuffer, 0, kBindingSize}, - {2, readonlyStorageBuffer, 0, kBindingSize}}); + {1, uniformBuffer, 0, kBindingSize}, + {2, storageBuffer, 0, kBindingSize}, + {3, readonlyStorageBuffer, 0, kBindingSize}}); // Dynamic offset + offset isn't larger than buffer size. // But with binding size, it will trigger OOB error. std::array offsets = {0, 256, 768}; diff --git a/src/tests/unittests/validation/StorageTextureValidationTests.cpp b/src/tests/unittests/validation/StorageTextureValidationTests.cpp index 30e9e65b6f..80957b087c 100644 --- a/src/tests/unittests/validation/StorageTextureValidationTests.cpp +++ b/src/tests/unittests/validation/StorageTextureValidationTests.cpp @@ -228,7 +228,7 @@ TEST_F(StorageTextureValidationTests, ComputePipeline) { utils::CreateShaderModule(device, utils::SingleShaderStage::Compute, R"( #version 450 layout(set = 0, binding = 0, rgba8) uniform readonly image2D image0; - layout(std430, set = 0, binding = 0) buffer Buf { uint buf; }; + layout(std430, set = 0, binding = 1) buffer Buf { uint buf; }; void main() { vec4 pixel = imageLoad(image0, ivec2(gl_LocalInvocationID.xy)); buf = uint(pixel.x);