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>

src/dawn_native/BindGroupLayout.cpp

@@ -233,22 +233,53 @@ namespace dawn_native {
                    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) {
+            const bool aIsBuffer = IsBufferBinding(a.type);
-                // Buffers with dynamic offsets should come before those without.
+            const bool bIsBuffer = IsBufferBinding(b.type);
-                // This makes it easy to iterate over the dynamic buffer bindings
+            if (aIsBuffer != bIsBuffer) {
-                // [0, dynamicBufferCount) during validation.
+                // Always place buffers first.
-                return a.hasDynamicOffset > b.hasDynamicOffset;
+                return aIsBuffer;
-            }
+            } else {
-            if (a.type != b.type) {
+                if (aIsBuffer) {
-                // Buffers have smaller type enums. They should be placed first.
+                    ASSERT(bIsBuffer);
-                return a.type < b.type;
+                    if (a.hasDynamicOffset != b.hasDynamicOffset) {
-            }
+                        // Buffers with dynamic offsets should come before those without.
-            if (a.binding != b.binding) {
+                        // This makes it easy to iterate over the dynamic buffer bindings
-                // Above we ensure that dynamic buffers are first. Now, ensure that bindings are in
+                        // [0, dynamicBufferCount) during validation.
-                // increasing order. This is because dynamic buffer offsets are applied in
+                        return a.hasDynamicOffset;
-                // increasing order of binding number.
+                    }
-                return a.binding < b.binding;
+                    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 @@ namespace dawn_native {
             BindingIndex lastBufferIndex = 0;
             BindingIndex firstNonBufferIndex = std::numeric_limits<BindingIndex>::max();
             for (BindingIndex i = 0; i < count; ++i) {
-                switch (bindings[i].type) {
+                if (IsBufferBinding(bindings[i].type)) {
-                    case wgpu::BindingType::UniformBuffer:
+                    lastBufferIndex = std::max(i, lastBufferIndex);
-                    case wgpu::BindingType::StorageBuffer:
+                } else {
-                    case wgpu::BindingType::ReadonlyStorageBuffer:
+                    firstNonBufferIndex = std::min(i, firstNonBufferIndex);
-                        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;
                 }
             }
 

src/tests/end2end/BindGroupTests.cpp

@@ -762,9 +762,13 @@ TEST_P(BindGroupTests, DynamicOffsetOrder) {
     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 @@ TEST_P(BindGroupTests, DynamicOffsetOrder) {
         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 @@ TEST_P(BindGroupTests, DynamicOffsetOrder) {
     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 {

src/tests/unittests/validation/BindGroupValidationTests.cpp

@@ -1012,11 +1012,13 @@ TEST_F(SetBindGroupValidationTest, BindingSizeOutOfBoundDynamicStorageBuffer) {
 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 @@ TEST_F(SetBindGroupValidationTest, DynamicOffsetOrder) {
     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},