Validate writable storage texture bindings don't alias

Followup of storage buffer bindings aliasing validation.

Bug: dawn:1642
Change-Id: I84bf33895320053630ed80d3503ff53d1eaa83b9
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/121420
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Shrek Shao <shrekshao@google.com>

src/dawn/native/CommandBufferStateTracker.cpp

@@ -17,6 +17,7 @@
 #include <optional>
 #include <type_traits>
 #include <utility>
+#include <variant>
 
 #include "dawn/common/Assert.h"
 #include "dawn/common/BitSetIterator.h"
@@ -53,7 +54,7 @@ std::optional<uint32_t> FindFirstUndersizedBuffer(
     return std::nullopt;
 }
 
-struct BufferBindingAliasingResult {
+struct BufferAliasing {
     struct Entry {
         BindGroupIndex bindGroupIndex;
         BindingIndex bindingIndex;
@@ -66,18 +67,42 @@ struct BufferBindingAliasingResult {
     Entry e1;
 };
 
-// TODO(dawn:1642): Find storage texture binding aliasing as well.
+struct TextureAliasing {
+    struct Entry {
+        BindGroupIndex bindGroupIndex;
+        BindingIndex bindingIndex;
+
+        uint32_t baseMipLevel;
+        uint32_t mipLevelCount;
+        uint32_t baseArrayLayer;
+        uint32_t arrayLayerCount;
+    };
+    Entry e0;
+    Entry e1;
+};
+
+using WritableBindingAliasingResult = std::variant<std::monostate, BufferAliasing, TextureAliasing>;
+
 template <typename Return>
 Return FindStorageBufferBindingAliasing(
     const PipelineLayoutBase* pipelineLayout,
     const ityp::array<BindGroupIndex, BindGroupBase*, kMaxBindGroups>& bindGroups,
-    const ityp::array<BindGroupIndex, std::vector<uint32_t>, kMaxBindGroups> dynamicOffsets) {
+    const ityp::array<BindGroupIndex, std::vector<uint32_t>, kMaxBindGroups>& dynamicOffsets) {
+    // If true, returns detailed validation error info. Otherwise simply returns if any binding
+    // aliasing is found.
+    constexpr bool kProduceDetails = std::is_same_v<Return, WritableBindingAliasingResult>;
+
     // Reduce the bindings array first to only preserve storage buffer bindings that could
     // potentially have ranges overlap.
-    // There can at most be 8 storage buffer bindings per shader stage.
-    StackVector<BufferBinding, 8> bindingsToCheck;
+    // There can at most be 8 storage buffer bindings (in default limits) per shader stage.
+    StackVector<BufferBinding, 8> storageBufferBindingsToCheck;
+    StackVector<std::pair<BindGroupIndex, BindingIndex>, 8> bufferBindingIndices;
 
-    StackVector<std::pair<BindGroupIndex, BindingIndex>, 8> bindingIndices;
+    // Reduce the bindings array first to only preserve writable storage texture bindings that could
+    // potentially have ranges overlap.
+    // There can at most be 8 storage texture bindings (in default limits) per shader stage.
+    StackVector<const TextureViewBase*, 8> storageTextureViewsToCheck;
+    StackVector<std::pair<BindGroupIndex, BindingIndex>, 8> textureBindingIndices;
 
     for (BindGroupIndex groupIndex : IterateBitSet(pipelineLayout->GetBindGroupLayoutsMask())) {
         BindGroupLayoutBase* bgl = bindGroups[groupIndex]->GetLayout();
@@ -107,55 +132,127 @@ Return FindStorageBufferBindingAliasing(
                 adjustedOffset += dynamicOffsets[groupIndex][static_cast<uint32_t>(bindingIndex)];
             }
 
-            bindingsToCheck->push_back(BufferBinding{
+            storageBufferBindingsToCheck->push_back(BufferBinding{
                 bufferBinding.buffer,
                 adjustedOffset,
                 bufferBinding.size,
             });
 
-            if constexpr (std::is_same_v<Return, std::optional<BufferBindingAliasingResult>>) {
-                bindingIndices->emplace_back(groupIndex, bindingIndex);
+            if constexpr (kProduceDetails) {
+                bufferBindingIndices->emplace_back(groupIndex, bindingIndex);
+            }
+        }
+
+        // TODO(dawn:1642): optimize: precompute start/end range of storage textures bindings.
+        for (BindingIndex bindingIndex{bgl->GetBufferCount()};
+             bindingIndex < bgl->GetBindingCount(); ++bindingIndex) {
+            const BindingInfo& bindingInfo = bgl->GetBindingInfo(bindingIndex);
+
+            if (bindingInfo.bindingType != BindingInfoType::StorageTexture) {
+                continue;
+            }
+
+            switch (bindingInfo.storageTexture.access) {
+                case wgpu::StorageTextureAccess::WriteOnly:
+                    break;
+                // Continue for other StorageTextureAccess type when we have any.
+                default:
+                    UNREACHABLE();
+            }
+
+            const TextureViewBase* textureView =
+                bindGroups[groupIndex]->GetBindingAsTextureView(bindingIndex);
+
+            storageTextureViewsToCheck->push_back(textureView);
+
+            if constexpr (kProduceDetails) {
+                textureBindingIndices->emplace_back(groupIndex, bindingIndex);
             }
         }
     }
 
-    // Iterate through each bindings to find if any writable storage bindings aliasing exists.
-    // Given that maxStorageBuffersPerShaderStage is 8,
-    // it doesn't seem too bad to do a nested loop check.
+    // Iterate through each buffer bindings to find if any writable storage bindings aliasing
+    // exists. Given that maxStorageBuffersPerShaderStage is 8, it doesn't seem too bad to do a
+    // nested loop check.
     // TODO(dawn:1642): Maybe do algorithm optimization from O(N^2) to O(N*logN).
-    for (size_t i = 0; i < bindingsToCheck->size(); i++) {
-        const auto& bufferBinding0 = bindingsToCheck[i];
+    for (size_t i = 0; i < storageBufferBindingsToCheck->size(); i++) {
+        const auto& bufferBinding0 = storageBufferBindingsToCheck[i];
 
-        for (size_t j = i + 1; j < bindingsToCheck->size(); j++) {
-            const auto& bufferBinding1 = bindingsToCheck[j];
+        for (size_t j = i + 1; j < storageBufferBindingsToCheck->size(); j++) {
+            const auto& bufferBinding1 = storageBufferBindingsToCheck[j];
 
             if (bufferBinding0.buffer != bufferBinding1.buffer) {
                 continue;
             }
 
-            if (bufferBinding0.offset <= bufferBinding1.offset + bufferBinding1.size - 1 &&
-                bufferBinding1.offset <= bufferBinding0.offset + bufferBinding0.size - 1) {
-                if constexpr (std::is_same_v<Return, bool>) {
+            if (RangesOverlap(
+                    bufferBinding0.offset, bufferBinding0.offset + bufferBinding0.size - 1,
+                    bufferBinding1.offset, bufferBinding1.offset + bufferBinding1.size - 1)) {
+                if constexpr (kProduceDetails) {
+                    return WritableBindingAliasingResult{BufferAliasing{
+                        {bufferBindingIndices[i].first, bufferBindingIndices[i].second,
+                         bufferBinding0.offset, bufferBinding0.size},
+                        {bufferBindingIndices[j].first, bufferBindingIndices[j].second,
+                         bufferBinding1.offset, bufferBinding1.size},
+                    }};
+                } else {
                     return true;
-                } else if constexpr (std::is_same_v<Return,
-                                                    std::optional<BufferBindingAliasingResult>>) {
-                    return BufferBindingAliasingResult{
-                        {bindingIndices[i].first, bindingIndices[i].second, bufferBinding0.offset,
-                         bufferBinding0.size},
-                        {bindingIndices[j].first, bindingIndices[j].second, bufferBinding1.offset,
-                         bufferBinding1.size},
-                    };
                 }
             }
         }
     }
 
-    if constexpr (std::is_same_v<Return, bool>) {
-        return false;
-    } else if constexpr (std::is_same_v<Return, std::optional<BufferBindingAliasingResult>>) {
-        return std::nullopt;
+    // Iterate through each texture views to find if any writable storage bindings aliasing exists.
+    // Given that maxStorageTexturesPerShaderStage is 8,
+    // it doesn't seem too bad to do a nested loop check.
+    // TODO(dawn:1642): Maybe do algorithm optimization from O(N^2) to O(N*logN).
+    for (size_t i = 0; i < storageTextureViewsToCheck->size(); i++) {
+        const TextureViewBase* textureView0 = storageTextureViewsToCheck[i];
+
+        ASSERT(textureView0->GetAspects() == Aspect::Color);
+
+        uint32_t baseMipLevel0 = textureView0->GetBaseMipLevel();
+        uint32_t mipLevelCount0 = textureView0->GetLevelCount();
+        uint32_t baseArrayLayer0 = textureView0->GetBaseArrayLayer();
+        uint32_t arrayLayerCount0 = textureView0->GetLayerCount();
+
+        for (size_t j = i + 1; j < storageTextureViewsToCheck->size(); j++) {
+            const TextureViewBase* textureView1 = storageTextureViewsToCheck[j];
+
+            if (textureView0->GetTexture() != textureView1->GetTexture()) {
+                continue;
+            }
+
+            ASSERT(textureView1->GetAspects() == Aspect::Color);
+
+            uint32_t baseMipLevel1 = textureView1->GetBaseMipLevel();
+            uint32_t mipLevelCount1 = textureView1->GetLevelCount();
+            uint32_t baseArrayLayer1 = textureView1->GetBaseArrayLayer();
+            uint32_t arrayLayerCount1 = textureView1->GetLayerCount();
+
+            if (RangesOverlap(baseMipLevel0, baseMipLevel0 + mipLevelCount0 - 1, baseMipLevel1,
+                              baseMipLevel1 + mipLevelCount1 - 1) &&
+                RangesOverlap(baseArrayLayer0, baseArrayLayer0 + arrayLayerCount0 - 1,
+                              baseArrayLayer1, baseArrayLayer1 + arrayLayerCount1 - 1)) {
+                if constexpr (kProduceDetails) {
+                    return WritableBindingAliasingResult{TextureAliasing{
+                        {textureBindingIndices[i].first, textureBindingIndices[i].second,
+                         baseMipLevel0, mipLevelCount0, baseArrayLayer0, arrayLayerCount0},
+                        {textureBindingIndices[j].first, textureBindingIndices[j].second,
+                         baseMipLevel1, mipLevelCount1, baseArrayLayer1, arrayLayerCount1},
+                    }};
+                } else {
+                    return true;
+                }
+            }
+        }
+    }
+
+    if constexpr (kProduceDetails) {
+        return WritableBindingAliasingResult();
+    } else {
+        return false;
     }
-    UNREACHABLE();
 }
 
 }  // namespace
@@ -396,7 +493,7 @@ MaybeError CommandBufferStateTracker::CheckMissingAspects(ValidationAspects aspe
 
     DAWN_INVALID_IF(aspects[VALIDATION_ASPECT_PIPELINE], "No pipeline set.");
 
-    if (DAWN_UNLIKELY(aspects[VALIDATION_ASPECT_INDEX_BUFFER])) {
+    if (aspects[VALIDATION_ASPECT_INDEX_BUFFER]) {
         DAWN_INVALID_IF(!mIndexBufferSet, "Index buffer was not set.");
 
         RenderPipelineBase* lastRenderPipeline = GetRenderPipeline();
@@ -436,7 +533,7 @@ MaybeError CommandBufferStateTracker::CheckMissingAspects(ValidationAspects aspe
                                      uint8_t(firstMissing), GetRenderPipeline());
     }
 
-    if (DAWN_UNLIKELY(aspects[VALIDATION_ASPECT_BIND_GROUPS])) {
+    if (aspects[VALIDATION_ASPECT_BIND_GROUPS]) {
         for (BindGroupIndex i : IterateBitSet(mLastPipelineLayout->GetBindGroupLayoutsMask())) {
             ASSERT(HasPipeline());
 
@@ -495,19 +592,39 @@ MaybeError CommandBufferStateTracker::CheckMissingAspects(ValidationAspects aspe
             }
         }
 
-        auto result = FindStorageBufferBindingAliasing<std::optional<BufferBindingAliasingResult>>(
+        auto result = FindStorageBufferBindingAliasing<WritableBindingAliasingResult>(
             mLastPipelineLayout, mBindgroups, mDynamicOffsets);
 
-        if (result) {
+        if (std::holds_alternative<BufferAliasing>(result)) {
+            const auto& a = std::get<BufferAliasing>(result);
             return DAWN_VALIDATION_ERROR(
-                "Writable storage buffer binding found between bind group index %u, binding index "
-                "%u, and bind group index %u, binding index %u, with overlapping ranges (offset: "
+                "Writable storage buffer binding aliasing found between bind group index %u, "
+                "binding index "
+                "%u, and bind group index %u, binding index %u, with overlapping ranges "
+                "(offset: "
                 "%u, size: %u) and (offset: %u, size: %u).",
-                static_cast<uint32_t>(result->e0.bindGroupIndex),
-                static_cast<uint32_t>(result->e0.bindingIndex),
-                static_cast<uint32_t>(result->e1.bindGroupIndex),
-                static_cast<uint32_t>(result->e1.bindingIndex), result->e0.offset, result->e0.size,
-                result->e1.offset, result->e1.size);
+                static_cast<uint32_t>(a.e0.bindGroupIndex),
+                static_cast<uint32_t>(a.e0.bindingIndex),
+                static_cast<uint32_t>(a.e1.bindGroupIndex),
+                static_cast<uint32_t>(a.e1.bindingIndex), a.e0.offset, a.e0.size, a.e1.offset,
+                a.e1.size);
+        } else {
+            ASSERT(std::holds_alternative<TextureAliasing>(result));
+            const auto& a = std::get<TextureAliasing>(result);
+            return DAWN_VALIDATION_ERROR(
+                "Writable storage texture binding aliasing found between bind group "
+                "index %u, binding index "
+                "%u, and bind group index %u, binding index %u, with subresources "
+                "(base mipmap level: "
+                "%u, mip level count: %u, base array layer: %u, array layer count: %u) and "
+                "(base mipmap level: %u, mip level count: "
+                "%u, base array layer: %u, array layer count: %u).",
+                static_cast<uint32_t>(a.e0.bindGroupIndex),
+                static_cast<uint32_t>(a.e0.bindingIndex),
+                static_cast<uint32_t>(a.e1.bindGroupIndex),
+                static_cast<uint32_t>(a.e1.bindingIndex), a.e0.baseMipLevel, a.e0.mipLevelCount,
+                a.e0.baseArrayLayer, a.e0.arrayLayerCount, a.e1.baseMipLevel, a.e1.mipLevelCount,
+                a.e1.baseArrayLayer, a.e1.arrayLayerCount);
         }
 
         // The chunk of code above should be similar to the one in |RecomputeLazyAspects|.

src/dawn/native/CommandValidation.cpp

@@ -21,6 +21,7 @@
 #include <utility>
 
 #include "dawn/common/BitSetIterator.h"
+#include "dawn/common/Numeric.h"
 #include "dawn/native/Adapter.h"
 #include "dawn/native/BindGroup.h"
 #include "dawn/native/Buffer.h"
@@ -115,10 +116,14 @@ MaybeError ValidateWriteBuffer(const DeviceBase* device,
 }
 
 bool IsRangeOverlapped(uint32_t startA, uint32_t startB, uint32_t length) {
-    uint32_t maxStart = std::max(startA, startB);
-    uint32_t minStart = std::min(startA, startB);
-    return static_cast<uint64_t>(minStart) + static_cast<uint64_t>(length) >
-           static_cast<uint64_t>(maxStart);
+    if (length < 1) {
+        return false;
+    }
+    return RangesOverlap<uint64_t>(
+        static_cast<uint64_t>(startA),
+        static_cast<uint64_t>(startA) + static_cast<uint64_t>(length) - 1,
+        static_cast<uint64_t>(startB),
+        static_cast<uint64_t>(startB) + static_cast<uint64_t>(length) - 1);
 }
 
 ResultOrError<uint64_t> ComputeRequiredBytesInCopy(const TexelBlockInfo& blockInfo,

src/dawn/native/Subresource.h

@@ -21,7 +21,7 @@
 namespace dawn::native {
 
 // Note: Subresource indices are computed by iterating the aspects in increasing order.
-// D3D12 uses these directly, so the order much match D3D12's indices.
+// D3D12 uses these directly, so the order must match D3D12's indices.
 //  - Depth/Stencil textures have Depth as Plane 0, and Stencil as Plane 1.
 enum class Aspect : uint8_t {
     None = 0x0,