Introduce SubresourceStorage (2/N): Merge

This CL adds the Merge() operation to SubresourceStorage() that allows
modifying the content of a storage with another storage.

Bug: dawn:441

Change-Id: I28e3cd7bc967056eda2c387b2b6e164eb370a241
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/35520
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>

src/dawn_native/SubresourceStorage.h

@@ -81,9 +81,23 @@ namespace dawn_native {
     //
     // T must be a copyable type that supports equality comparison with ==.
     //
-    // TODO(cwallez@chromium.org): Add the Merge() operation.
+    // The implementation of functions in this file can have a lot of control flow and corner cases
+    // so each modification should come with extensive tests and ensure 100% code coverage of the
+    // modified functions. See instructions at
+    // https://chromium.googlesource.com/chromium/src/+/master/docs/testing/code_coverage.md#local-coverage-script
+    // to run the test with code coverage. A command line that worked in the past (with the right
+    // GN args for the out/coverage directory in a Chromium checkout) is:
+    //
+    /*
+       python tools/code_coverage/coverage.py dawn_unittests -b out/coverage -o out/report -c \
+           "out/coverage/dawn_unittests --gtest_filter=SubresourceStorage\*" -f \
+           third_party/dawn/src/dawn_native
+    */
+    //
     // TODO(cwallez@chromium.org): Inline the storage for aspects to avoid allocating when
     // possible.
+    // TODO(cwallez@chromium.org): Make the recompression optional, the calling code should know
+    // if recompression can happen or not in Update() and Merge()
     template <typename T>
     class SubresourceStorage {
       public:
@@ -98,7 +112,7 @@ namespace dawn_native {
         // same for multiple subresources.
         const T& Get(Aspect aspect, uint32_t arrayLayer, uint32_t mipLevel) const;
 
-        // Given an iterateFunc that's a function or function-like objet that call be called with
+        // Given an iterateFunc that's a function or function-like objet that can be called with
         // arguments of type (const SubresourceRange& range, const T& data) and returns void,
         // calls it with aggregate ranges if possible, such that each subresource is part of
         // exactly one of the ranges iterateFunc is called with (and obviously data is the value
@@ -110,7 +124,7 @@ namespace dawn_native {
         template <typename F>
         void Iterate(F&& iterateFunc) const;
 
-        // Given an updateFunc that's a function or function-like objet that call be called with
+        // Given an updateFunc that's a function or function-like objet that can be called with
         // arguments of type (const SubresourceRange& range, T* data) and returns void,
         // calls it with ranges that in aggregate form `range` and pass for each of the
         // sub-ranges a pointer to modify the value for that sub-range. For example:
@@ -125,10 +139,25 @@ namespace dawn_native {
         template <typename F>
         void Update(const SubresourceRange& range, F&& updateFunc);
 
+        // Given a mergeFunc that's a function or a function-like object that can be called with
+        // arguments of type (const SubresourceRange& range, T* data, const U& otherData) and
+        // returns void, calls it with ranges that in aggregate form the full resources and pass
+        // for each of the sub-ranges a pointer to modify the value for that sub-range and the
+        // corresponding value from other for that sub-range. For example:
+        //
+        //   subresources.Merge(otherUsages,
+        //       [](const SubresourceRange&, T* data, const T& otherData) {
+        //          *data |= otherData;
+        //       });
+        //
+        // /!\ WARNING: mergeFunc should never use range to compute the update to data otherwise
+        // your code is likely to break when compression happens. Range should only be used for
+        // side effects like using it to compute a Vulkan pipeline barrier.
+        template <typename U, typename F>
+        void Merge(const SubresourceStorage<U>& other, F&& mergeFunc);
+
         // Other operations to consider:
         //
-        //  - Merge(Range, SubresourceStorage<U>, mergeFunc) that takes the values from the other
-        //    storage and modifies the value of the current storage with it.
         //  - UpdateTo(Range, T) that updates the range to a constant value.
 
         // Methods to query the internal state of SubresourceStorage for testing.
@@ -139,6 +168,9 @@ namespace dawn_native {
         bool IsLayerCompressedForTesting(Aspect aspect, uint32_t layer) const;
 
       private:
+        template <typename U>
+        friend class SubresourceStorage;
+
         void DecompressAspect(uint32_t aspectIndex);
         void RecompressAspect(uint32_t aspectIndex);
 
@@ -254,6 +286,63 @@ namespace dawn_native {
         }
     }
 
+    template <typename T>
+    template <typename U, typename F>
+    void SubresourceStorage<T>::Merge(const SubresourceStorage<U>& other, F&& mergeFunc) {
+        ASSERT(mAspects == other.mAspects);
+        ASSERT(mArrayLayerCount == other.mArrayLayerCount);
+        ASSERT(mMipLevelCount == other.mMipLevelCount);
+
+        for (Aspect aspect : IterateEnumMask(mAspects)) {
+            uint32_t aspectIndex = GetAspectIndex(aspect);
+
+            // If the other storage's aspect is compressed we don't need to decompress anything
+            // in `this` and can just iterate through it, merging with `other`'s constant value for
+            // the aspect. For code simplicity this can be done with a call to Update().
+            if (other.mAspectCompressed[aspectIndex]) {
+                const U& otherData = other.Data(aspectIndex);
+                Update(SubresourceRange::MakeFull(aspect, mArrayLayerCount, mMipLevelCount),
+                       [&](const SubresourceRange& subrange, T* data) {
+                           mergeFunc(subrange, data, otherData);
+                       });
+                continue;
+            }
+
+            // Other doesn't have the aspect compressed so we must do at least per-layer merging.
+            if (mAspectCompressed[aspectIndex]) {
+                DecompressAspect(aspectIndex);
+            }
+
+            for (uint32_t layer = 0; layer < mArrayLayerCount; layer++) {
+                // Similarly to above, use a fast path if other's layer is compressed.
+                if (other.LayerCompressed(aspectIndex, layer)) {
+                    const U& otherData = other.Data(aspectIndex, layer);
+                    Update(GetFullLayerRange(aspect, layer),
+                           [&](const SubresourceRange& subrange, T* data) {
+                               mergeFunc(subrange, data, otherData);
+                           });
+                    continue;
+                }
+
+                // Sad case, other is decompressed for this layer, do per-level merging.
+                if (LayerCompressed(aspectIndex, layer)) {
+                    DecompressLayer(aspectIndex, layer);
+                }
+
+                for (uint32_t level = 0; level < mMipLevelCount; level++) {
+                    SubresourceRange updateRange =
+                        SubresourceRange::MakeSingle(aspect, layer, level);
+                    mergeFunc(updateRange, &Data(aspectIndex, layer, level),
+                              other.Data(aspectIndex, layer, level));
+                }
+
+                RecompressLayer(aspectIndex, layer);
+            }
+
+            RecompressAspect(aspectIndex);
+        }
+    }
+
     template <typename T>
     template <typename F>
     void SubresourceStorage<T>::Iterate(F&& iterateFunc) const {

src/tests/unittests/SubresourceStorageTests.cpp

@@ -49,6 +49,19 @@ struct FakeStorage {
         }
     }
 
+    template <typename U, typename F>
+    void Merge(const SubresourceStorage<U>& other, F&& mergeFunc) {
+        for (Aspect aspect : IterateEnumMask(mAspects)) {
+            for (uint32_t layer = 0; layer < mArrayLayerCount; layer++) {
+                for (uint32_t level = 0; level < mMipLevelCount; level++) {
+                    SubresourceRange range = SubresourceRange::MakeSingle(aspect, layer, level);
+                    mergeFunc(range, &mData[GetDataIndex(aspect, layer, level)],
+                              other.Get(aspect, layer, level));
+                }
+            }
+        }
+    }
+
     const T& Get(Aspect aspect, uint32_t arrayLayer, uint32_t mipLevel) const {
         return mData[GetDataIndex(aspect, arrayLayer, mipLevel)];
     }
@@ -352,15 +365,15 @@ TEST(SubresourceStorageTest, UpdateTwoBand) {
     CheckLayerCompressed(s, Aspect::Stencil, 3, true);
 
     {
-        SubresourceRange range(Aspect::Depth | Aspect::Stencil, {0, kLayers}, {5, 2});
+        SubresourceRange range(Aspect::Depth, {0, kLayers}, {5, 2});
         CallUpdateOnBoth(&s, &f, range, [](const SubresourceRange&, int* data) { *data *= 3; });
     }
 
-    // The layers had to be decompressed.
+    // The layers had to be decompressed in depth
     CheckLayerCompressed(s, Aspect::Depth, 2, false);
     CheckLayerCompressed(s, Aspect::Depth, 3, false);
-    CheckLayerCompressed(s, Aspect::Stencil, 2, false);
+    CheckLayerCompressed(s, Aspect::Stencil, 2, true);
-    CheckLayerCompressed(s, Aspect::Stencil, 3, false);
+    CheckLayerCompressed(s, Aspect::Stencil, 3, true);
 
     // Update completely. Without a single value recompression shouldn't happen.
     {
@@ -445,6 +458,172 @@ TEST(SubresourceStorageTest, UpdateLevel0sHappenToMatch) {
     CheckLayerCompressed(s, Aspect::Color, 1, false);
 }
 
+// The tests for Merge() all follow the same as the Update() tests except that they use Update()
+// to set up the test storages.
+
+// Similar to CallUpdateOnBoth but for Merge
+template <typename T, typename U, typename F>
+void CallMergeOnBoth(SubresourceStorage<T>* s,
+                     FakeStorage<T>* f,
+                     const SubresourceStorage<U>& other,
+                     F&& mergeFunc) {
+    RangeTracker tracker(*s);
+
+    s->Merge(other, [&](const SubresourceRange& range, T* data, const U& otherData) {
+        tracker.Track(range);
+        mergeFunc(range, data, otherData);
+    });
+    f->Merge(other, mergeFunc);
+
+    tracker.CheckTrackedExactly(
+        SubresourceRange::MakeFull(f->mAspects, f->mArrayLayerCount, f->mMipLevelCount));
+    f->CheckSameAs(*s);
+}
+
+// Test merging two fully compressed single-aspect resources.
+TEST(SubresourceStorageTest, MergeFullWithFullSingleAspect) {
+    SubresourceStorage<int> s(Aspect::Color, 4, 6);
+    FakeStorage<int> f(Aspect::Color, 4, 6);
+
+    // Merge the whole resource in a single call.
+    SubresourceStorage<bool> other(Aspect::Color, 4, 6, true);
+    CallMergeOnBoth(&s, &f, other, [](const SubresourceRange&, int* data, bool other) {
+        if (other) {
+            *data = 13;
+        }
+    });
+
+    CheckAspectCompressed(s, Aspect::Color, true);
+}
+
+// Test merging two fully compressed multi-aspect resources.
+TEST(SubresourceStorageTest, MergeFullWithFullMultiAspect) {
+    SubresourceStorage<int> s(Aspect::Depth | Aspect::Stencil, 6, 7);
+    FakeStorage<int> f(Aspect::Depth | Aspect::Stencil, 6, 7);
+
+    // Merge the whole resource in a single call.
+    SubresourceStorage<bool> other(Aspect::Depth | Aspect::Stencil, 6, 7, true);
+    CallMergeOnBoth(&s, &f, other, [](const SubresourceRange&, int* data, bool other) {
+        if (other) {
+            *data = 13;
+        }
+    });
+
+    CheckAspectCompressed(s, Aspect::Depth, true);
+    CheckAspectCompressed(s, Aspect::Stencil, true);
+}
+
+// Test merging a fully compressed resource in a resource with the "cross band" pattern.
+//  - The first band is full layers [2, 3] on both aspects
+//  - The second band is full mips [5, 6] on one aspect.
+// This provides coverage of using a single piece of data from `other` to update all of `s`
+TEST(SubresourceStorageTest, MergeFullInTwoBand) {
+    const uint32_t kLayers = 5;
+    const uint32_t kLevels = 9;
+    SubresourceStorage<int> s(Aspect::Depth | Aspect::Stencil, kLayers, kLevels);
+    FakeStorage<int> f(Aspect::Depth | Aspect::Stencil, kLayers, kLevels);
+
+    // Update the two bands
+    {
+        SubresourceRange range(Aspect::Depth | Aspect::Stencil, {2, 2}, {0, kLevels});
+        CallUpdateOnBoth(&s, &f, range, [](const SubresourceRange&, int* data) { *data += 3; });
+    }
+    {
+        SubresourceRange range(Aspect::Depth, {0, kLayers}, {5, 2});
+        CallUpdateOnBoth(&s, &f, range, [](const SubresourceRange&, int* data) { *data += 5; });
+    }
+
+    // Merge the fully compressed resource.
+    SubresourceStorage<int> other(Aspect::Depth | Aspect::Stencil, kLayers, kLevels, 17);
+    CallMergeOnBoth(&s, &f, other,
+                    [](const SubresourceRange&, int* data, int other) { *data += other; });
+
+    // The layers traversed by the mip band are still uncompressed.
+    CheckLayerCompressed(s, Aspect::Depth, 1, false);
+    CheckLayerCompressed(s, Aspect::Depth, 2, false);
+    CheckLayerCompressed(s, Aspect::Depth, 3, false);
+    CheckLayerCompressed(s, Aspect::Depth, 4, false);
+
+    // Stencil is decompressed but all its layers are still compressed because there wasn't the mip
+    // band.
+    CheckAspectCompressed(s, Aspect::Stencil, false);
+    CheckLayerCompressed(s, Aspect::Stencil, 1, true);
+    CheckLayerCompressed(s, Aspect::Stencil, 2, true);
+    CheckLayerCompressed(s, Aspect::Stencil, 3, true);
+    CheckLayerCompressed(s, Aspect::Stencil, 4, true);
+}
+// Test the reverse, mergign two-bands in a full resource. This provides coverage for decompressing
+// aspects / and partilly layers to match the compression of `other`
+TEST(SubresourceStorageTest, MergeTwoBandInFull) {
+    const uint32_t kLayers = 5;
+    const uint32_t kLevels = 9;
+    SubresourceStorage<int> s(Aspect::Depth | Aspect::Stencil, kLayers, kLevels, 75);
+    FakeStorage<int> f(Aspect::Depth | Aspect::Stencil, kLayers, kLevels, 75);
+
+    // Update the two bands
+    SubresourceStorage<int> other(Aspect::Depth | Aspect::Stencil, kLayers, kLevels);
+    {
+        SubresourceRange range(Aspect::Depth | Aspect::Stencil, {2, 2}, {0, kLevels});
+        other.Update(range, [](const SubresourceRange&, int* data) { *data += 3; });
+    }
+    {
+        SubresourceRange range(Aspect::Depth, {0, kLayers}, {5, 2});
+        other.Update(range, [](const SubresourceRange&, int* data) { *data += 5; });
+    }
+
+    // Merge the fully compressed resource.
+    CallMergeOnBoth(&s, &f, other,
+                    [](const SubresourceRange&, int* data, int other) { *data += other; });
+
+    // The layers traversed by the mip band are still uncompressed.
+    CheckLayerCompressed(s, Aspect::Depth, 1, false);
+    CheckLayerCompressed(s, Aspect::Depth, 2, false);
+    CheckLayerCompressed(s, Aspect::Depth, 3, false);
+    CheckLayerCompressed(s, Aspect::Depth, 4, false);
+
+    // Stencil is decompressed but all its layers are still compressed because there wasn't the mip
+    // band.
+    CheckAspectCompressed(s, Aspect::Stencil, false);
+    CheckLayerCompressed(s, Aspect::Stencil, 1, true);
+    CheckLayerCompressed(s, Aspect::Stencil, 2, true);
+    CheckLayerCompressed(s, Aspect::Stencil, 3, true);
+    CheckLayerCompressed(s, Aspect::Stencil, 4, true);
+}
+
+// Test merging storage with a layer band in a stipple patterned storage. This provide coverage
+// for the code path that uses the same layer data for other multiple times.
+TEST(SubresourceStorageTest, MergeLayerBandInStipple) {
+    const uint32_t kLayers = 3;
+    const uint32_t kLevels = 5;
+
+    SubresourceStorage<int> s(Aspect::Color, kLayers, kLevels);
+    FakeStorage<int> f(Aspect::Color, kLayers, kLevels);
+    SubresourceStorage<int> other(Aspect::Color, kLayers, kLevels);
+
+    for (uint32_t layer = 0; layer < kLayers; layer++) {
+        for (uint32_t level = 0; level < kLevels; level++) {
+            if ((layer + level) % 2 == 0) {
+                SubresourceRange range = SubresourceRange::MakeSingle(Aspect::Color, layer, level);
+                CallUpdateOnBoth(&s, &f, range,
+                                 [](const SubresourceRange&, int* data) { *data += 17; });
+            }
+        }
+        if (layer % 2 == 0) {
+            other.Update({Aspect::Color, {layer, 1}, {0, kLevels}},
+                         [](const SubresourceRange&, int* data) { *data += 8; });
+        }
+    }
+
+    // Merge the band in the stipple.
+    CallMergeOnBoth(&s, &f, other,
+                    [](const SubresourceRange&, int* data, int other) { *data += other; });
+
+    // None of the resulting layers are compressed.
+    CheckLayerCompressed(s, Aspect::Color, 0, false);
+    CheckLayerCompressed(s, Aspect::Color, 1, false);
+    CheckLayerCompressed(s, Aspect::Color, 2, false);
+}
+
 // Bugs found while testing:
 //  - mLayersCompressed not initialized to true.
 //  - DecompressLayer setting Compressed to true instead of false.