Ensure dynamic buffer offset bindings are sorted in increasing order
A previous CL sorted bindings by binding number, but bindings were first
sorted by type. This means a bind group layout with mixed dynamic
storage and uniform buffers would not always have all dynamic bindings
in increasing order. Instead, it would be strictly increasing within
each section of uniform/storage buffers. This CL corrects the issue
by first sorting dynamic buffers by binding number.
Bug: dawn:408
Change-Id: I3689eb64ad8aa8768cebe266eebcba75a21894ce
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/22303
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>
diff --git a/src/dawn_native/BindGroupLayout.cpp b/src/dawn_native/BindGroupLayout.cpp
index f1156d5..a7d19fb 100644
--- a/src/dawn_native/BindGroupLayout.cpp
+++ b/src/dawn_native/BindGroupLayout.cpp
@@ -233,22 +233,53 @@
a.storageTextureFormat != b.storageTextureFormat;
}
+ bool IsBufferBinding(wgpu::BindingType bindingType) {
+ switch (bindingType) {
+ case wgpu::BindingType::UniformBuffer:
+ case wgpu::BindingType::StorageBuffer:
+ case wgpu::BindingType::ReadonlyStorageBuffer:
+ return true;
+ case wgpu::BindingType::SampledTexture:
+ case wgpu::BindingType::Sampler:
+ case wgpu::BindingType::ComparisonSampler:
+ case wgpu::BindingType::StorageTexture:
+ case wgpu::BindingType::ReadonlyStorageTexture:
+ case wgpu::BindingType::WriteonlyStorageTexture:
+ return false;
+ default:
+ UNREACHABLE();
+ return false;
+ }
+ }
+
bool SortBindingsCompare(const BindGroupLayoutEntry& a, const BindGroupLayoutEntry& 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.binding != b.binding) {
- // Above we ensure that dynamic buffers are first. Now, ensure that bindings are in
- // increasing order. This is because dynamic buffer offsets are applied in
- // increasing order of binding number.
- return a.binding < b.binding;
+ const bool aIsBuffer = IsBufferBinding(a.type);
+ const bool bIsBuffer = IsBufferBinding(b.type);
+ if (aIsBuffer != bIsBuffer) {
+ // Always place buffers first.
+ return aIsBuffer;
+ } else {
+ if (aIsBuffer) {
+ ASSERT(bIsBuffer);
+ 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;
+ }
+ if (a.hasDynamicOffset) {
+ ASSERT(b.hasDynamicOffset);
+ ASSERT(a.binding != b.binding);
+ // Above, we ensured that dynamic buffers are first. Now, ensure that
+ // dynamic buffer bindings are in increasing order. This is because dynamic
+ // buffer offsets are applied in increasing order of binding number.
+ return a.binding < b.binding;
+ }
+ }
+ // Otherwise, sort by type.
+ if (a.type != b.type) {
+ return a.type < b.type;
+ }
}
if (a.visibility != b.visibility) {
return a.visibility < b.visibility;
@@ -276,23 +307,10 @@
BindingIndex lastBufferIndex = 0;
BindingIndex firstNonBufferIndex = std::numeric_limits<BindingIndex>::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::ComparisonSampler:
- case wgpu::BindingType::StorageTexture:
- case wgpu::BindingType::ReadonlyStorageTexture:
- case wgpu::BindingType::WriteonlyStorageTexture:
- firstNonBufferIndex = std::min(i, firstNonBufferIndex);
- break;
- default:
- UNREACHABLE();
- break;
+ if (IsBufferBinding(bindings[i].type)) {
+ lastBufferIndex = std::max(i, lastBufferIndex);
+ } else {
+ firstNonBufferIndex = std::min(i, firstNonBufferIndex);
}
}
diff --git a/src/tests/end2end/BindGroupTests.cpp b/src/tests/end2end/BindGroupTests.cpp
index 0584fbf..e0f3a53 100644
--- a/src/tests/end2end/BindGroupTests.cpp
+++ b/src/tests/end2end/BindGroupTests.cpp
@@ -762,9 +762,13 @@
bufferDescriptor.usage = wgpu::BufferUsage::Storage | wgpu::BufferUsage::CopyDst;
wgpu::Buffer buffer0 = device.CreateBuffer(&bufferDescriptor);
- wgpu::Buffer buffer2 = device.CreateBuffer(&bufferDescriptor);
wgpu::Buffer buffer3 = device.CreateBuffer(&bufferDescriptor);
+ // This test uses both storage and uniform buffers to ensure buffer bindings are sorted first by
+ // binding number before type.
+ bufferDescriptor.usage = wgpu::BufferUsage::Uniform | wgpu::BufferUsage::CopyDst;
+ wgpu::Buffer buffer2 = device.CreateBuffer(&bufferDescriptor);
+
// Populate the values
buffer0.SetSubData(offsets[0], sizeof(uint32_t), &values[0]);
buffer2.SetSubData(offsets[1], sizeof(uint32_t), &values[1]);
@@ -780,7 +784,7 @@
device, {
{3, wgpu::ShaderStage::Compute, wgpu::BindingType::ReadonlyStorageBuffer, true},
{0, wgpu::ShaderStage::Compute, wgpu::BindingType::ReadonlyStorageBuffer, true},
- {2, wgpu::ShaderStage::Compute, wgpu::BindingType::ReadonlyStorageBuffer, true},
+ {2, wgpu::ShaderStage::Compute, wgpu::BindingType::UniformBuffer, true},
{4, wgpu::ShaderStage::Compute, wgpu::BindingType::StorageBuffer},
});
wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, bgl,
@@ -795,7 +799,7 @@
pipelineDescriptor.computeStage.module =
utils::CreateShaderModule(device, utils::SingleShaderStage::Compute, R"(
#version 450
- layout(std430, set = 0, binding = 2) readonly buffer Buffer2 {
+ layout(std140, set = 0, binding = 2) uniform Buffer2 {
uint value2;
};
layout(std430, set = 0, binding = 3) readonly buffer Buffer3 {
diff --git a/src/tests/unittests/validation/BindGroupValidationTests.cpp b/src/tests/unittests/validation/BindGroupValidationTests.cpp
index f84e07c..a08dfc4 100644
--- a/src/tests/unittests/validation/BindGroupValidationTests.cpp
+++ b/src/tests/unittests/validation/BindGroupValidationTests.cpp
@@ -1012,11 +1012,13 @@
TEST_F(SetBindGroupValidationTest, DynamicOffsetOrder) {
// Note: The order of the binding numbers of the bind group and bind group layout are
// intentionally different and not in increasing order.
+ // This test uses both storage and uniform buffers to ensure buffer bindings are sorted first by
+ // binding number before type.
wgpu::BindGroupLayout bgl = utils::MakeBindGroupLayout(
device, {
{3, wgpu::ShaderStage::Compute, wgpu::BindingType::ReadonlyStorageBuffer, true},
{0, wgpu::ShaderStage::Compute, wgpu::BindingType::ReadonlyStorageBuffer, true},
- {2, wgpu::ShaderStage::Compute, wgpu::BindingType::ReadonlyStorageBuffer, true},
+ {2, wgpu::ShaderStage::Compute, wgpu::BindingType::UniformBuffer, true},
});
// Create buffers which are 3x, 2x, and 1x the size of the minimum buffer offset, plus 4 bytes
@@ -1028,7 +1030,7 @@
wgpu::Buffer buffer2x =
CreateBuffer(2 * kMinDynamicBufferOffsetAlignment + 4, wgpu::BufferUsage::Storage);
wgpu::Buffer buffer1x =
- CreateBuffer(1 * kMinDynamicBufferOffsetAlignment + 4, wgpu::BufferUsage::Storage);
+ CreateBuffer(1 * kMinDynamicBufferOffsetAlignment + 4, wgpu::BufferUsage::Uniform);
wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, bgl,
{
{0, buffer3x, 0, 4},
