Rephrase Format::aspect enum as an enum class mask

Format::aspect should be a mask so that it is easier to iterate over
and test if an aspect is present.

This CL also re-exports wgpu's EnumClassBitMask helpers in dawn_native.
It also adds an EnumMaskIterator which wraps BitSetIterator to allow
iterating over the enums in an enum mask.

Bug: dawn:439
Change-Id: I08180a45b27c6031e2f80b0fa1801716677fd813
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/24682
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Jiawei Shao <jiawei.shao@intel.com>
Commit-Queue: Austin Eng <enga@chromium.org>

src/dawn_native/BUILD.gn

@@ -192,6 +192,8 @@ source_set("dawn_native_sources") {
     "DynamicUploader.h",
     "EncodingContext.cpp",
     "EncodingContext.h",
+    "EnumClassBitmasks.h",
+    "EnumMaskIterator.h",
     "Error.cpp",
     "Error.h",
     "ErrorData.cpp",

src/dawn_native/CMakeLists.txt

@@ -67,6 +67,8 @@ target_sources(dawn_native PRIVATE
     "DynamicUploader.h"
     "EncodingContext.cpp"
     "EncodingContext.h"
+    "EnumClassBitmasks.h"
+    "EnumMaskIterator.h"
     "Error.cpp"
     "Error.h"
     "ErrorData.cpp"

src/dawn_native/EnumClassBitmasks.h

@@ -0,0 +1,41 @@
+// 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_ENUMCLASSBITMASK_H_
+#define DAWNNATIVE_ENUMCLASSBITMASK_H_
+
+#include "dawn/EnumClassBitmasks.h"
+
+namespace dawn_native {
+
+    // EnumClassBitmmasks is a WebGPU helper in the wgpu:: namespace.
+    // Re-export it in the dawn_native namespace.
+
+    // Specify this for usage with EnumMaskIterator
+    template <typename T>
+    struct EnumBitmaskSize {
+        static constexpr unsigned value = 0;
+    };
+
+    using wgpu::operator|;
+    using wgpu::operator&;
+    using wgpu::operator^;
+    using wgpu::operator~;
+    using wgpu::operator&=;
+    using wgpu::operator|=;
+    using wgpu::operator^=;
+
+}  // namespace dawn_native
+
+#endif  // DAWNNATIVE_ENUMCLASSBITMASK_H_

src/dawn_native/EnumMaskIterator.h

@@ -0,0 +1,80 @@
+// 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_ENUMMASKITERATOR_H_
+#define DAWNNATIVE_ENUMMASKITERATOR_H_
+
+#include "common/BitSetIterator.h"
+#include "dawn_native/EnumClassBitmasks.h"
+
+namespace dawn_native {
+
+    template <typename T>
+    class EnumMaskIterator final {
+        static constexpr size_t N = EnumBitmaskSize<T>::value;
+        static_assert(N > 0, "");
+
+        using U = std::underlying_type_t<T>;
+
+      public:
+        EnumMaskIterator(const T& mask) : mBitSetIterator(std::bitset<N>(static_cast<U>(mask))) {
+        }
+
+        class Iterator final {
+          public:
+            Iterator(const typename BitSetIterator<N, U>::Iterator& iter) : mIter(iter) {
+            }
+
+            Iterator& operator++() {
+                ++mIter;
+                return *this;
+            }
+
+            bool operator==(const Iterator& other) const {
+                return mIter == other.mIter;
+            }
+
+            bool operator!=(const Iterator& other) const {
+                return mIter != other.mIter;
+            }
+
+            T operator*() const {
+                U value = *mIter;
+                return static_cast<T>(U(1) << value);
+            }
+
+          private:
+            typename BitSetIterator<N, U>::Iterator mIter;
+        };
+
+        Iterator begin() const {
+            return Iterator(mBitSetIterator.begin());
+        }
+
+        Iterator end() const {
+            return Iterator(mBitSetIterator.end());
+        }
+
+      private:
+        BitSetIterator<N, U> mBitSetIterator;
+    };
+
+    template <typename T>
+    EnumMaskIterator<T> IterateEnumMask(const T& mask) {
+        return EnumMaskIterator<T>(mask);
+    }
+
+}  // namespace dawn_native
+
+#endif  // DAWNNATIVE_ENUMMASKITERATOR_H_

src/dawn_native/Format.cpp

@@ -13,8 +13,10 @@
 // limitations under the License.
 
 #include "dawn_native/Format.h"
+
 #include "dawn_native/Device.h"
 #include "dawn_native/Extensions.h"
+#include "dawn_native/Texture.h"
 
 #include <bitset>
 
@@ -58,19 +60,19 @@ namespace dawn_native {
     }
 
     bool Format::IsColor() const {
-        return aspect == Aspect::Color;
+        return aspects == Aspect::Color;
     }
 
     bool Format::HasDepth() const {
-        return aspect == Depth || aspect == DepthStencil;
+        return (aspects & Aspect::Depth) != 0;
     }
 
     bool Format::HasStencil() const {
-        return aspect == Stencil || aspect == DepthStencil;
+        return (aspects & Aspect::Stencil) != 0;
     }
 
     bool Format::HasDepthOrStencil() const {
-        return aspect != Color;
+        return (aspects & (Aspect::Depth | Aspect::Stencil)) != 0;
     }
 
     bool Format::HasComponentType(Type componentType) const {
@@ -98,7 +100,6 @@ namespace dawn_native {
         std::bitset<kKnownFormatCount> formatsSet;
 
         using Type = Format::Type;
-        using Aspect = Format::Aspect;
 
         auto AddFormat = [&table, &formatsSet](Format format) {
             size_t index = ComputeFormatIndex(format.format);
@@ -121,7 +122,7 @@ namespace dawn_native {
             internalFormat.isCompressed = false;
             internalFormat.isSupported = true;
             internalFormat.supportsStorageUsage = supportsStorageUsage;
-            internalFormat.aspect = Aspect::Color;
+            internalFormat.aspects = Aspect::Color;
             internalFormat.type = type;
             internalFormat.blockByteSize = byteSize;
             internalFormat.blockWidth = 1;
@@ -129,7 +130,7 @@ namespace dawn_native {
             AddFormat(internalFormat);
         };
 
-        auto AddDepthStencilFormat = [&AddFormat](wgpu::TextureFormat format, Format::Aspect aspect,
+        auto AddDepthStencilFormat = [&AddFormat](wgpu::TextureFormat format, Aspect aspects,
                                                   uint32_t byteSize) {
             Format internalFormat;
             internalFormat.format = format;
@@ -137,7 +138,7 @@ namespace dawn_native {
             internalFormat.isCompressed = false;
             internalFormat.isSupported = true;
             internalFormat.supportsStorageUsage = false;
-            internalFormat.aspect = aspect;
+            internalFormat.aspects = aspects;
             internalFormat.type = Type::Other;
             internalFormat.blockByteSize = byteSize;
             internalFormat.blockWidth = 1;
@@ -153,7 +154,7 @@ namespace dawn_native {
             internalFormat.isCompressed = false;
             internalFormat.isSupported = true;
             internalFormat.supportsStorageUsage = false;
-            internalFormat.aspect = Aspect::Depth;
+            internalFormat.aspects = Aspect::Depth;
             internalFormat.type = type;
             internalFormat.blockByteSize = byteSize;
             internalFormat.blockWidth = 1;
@@ -169,7 +170,7 @@ namespace dawn_native {
             internalFormat.isCompressed = true;
             internalFormat.isSupported = isSupported;
             internalFormat.supportsStorageUsage = false;
-            internalFormat.aspect = Aspect::Color;
+            internalFormat.aspects = Aspect::Color;
             internalFormat.type = Type::Float;
             internalFormat.blockByteSize = byteSize;
             internalFormat.blockWidth = width;
@@ -232,7 +233,8 @@ namespace dawn_native {
         AddDepthStencilFormat(wgpu::TextureFormat::Depth24Plus, Aspect::Depth, 4);
         // TODO(cwallez@chromium.org): It isn't clear if this format should be copyable
         // because its size isn't well defined, is it 4, 5 or 8?
-        AddDepthStencilFormat(wgpu::TextureFormat::Depth24PlusStencil8, Aspect::DepthStencil, 4);
+        AddDepthStencilFormat(wgpu::TextureFormat::Depth24PlusStencil8,
+                              Aspect::Depth | Aspect::Stencil, 4);
 
         // BC compressed formats
         bool isBCFormatSupported = device->IsExtensionEnabled(Extension::TextureCompressionBC);

src/dawn_native/Format.h

@@ -17,12 +17,16 @@
 
 #include "dawn_native/dawn_platform.h"
 
+#include "common/ityp_bitset.h"
 #include "dawn_native/Error.h"
 
+#include "dawn_native/EnumClassBitmasks.h"
+
 #include <array>
 
 namespace dawn_native {
 
+    enum class Aspect : uint8_t;
     class DeviceBase;
 
     // The number of formats Dawn knows about. Asserts in BuildFormatTable ensure that this is the
@@ -31,14 +35,7 @@ namespace dawn_native {
 
     // A wgpu::TextureFormat along with all the information about it necessary for validation.
     struct Format {
-        enum Aspect {
+        enum class Type {
-            Color,
-            Depth,
-            Stencil,
-            DepthStencil,
-        };
-
-        enum Type {
             Float,
             Sint,
             Uint,
@@ -51,8 +48,8 @@ namespace dawn_native {
         // A format can be known but not supported because it is part of a disabled extension.
         bool isSupported;
         bool supportsStorageUsage;
-        Aspect aspect;
         Type type;
+        Aspect aspects;
 
         uint32_t blockByteSize;
         uint32_t blockWidth;

src/dawn_native/ShaderModule.cpp

@@ -39,14 +39,14 @@ namespace dawn_native {
         Format::Type SpirvCrossBaseTypeToFormatType(spirv_cross::SPIRType::BaseType spirvBaseType) {
             switch (spirvBaseType) {
                 case spirv_cross::SPIRType::Float:
-                    return Format::Float;
+                    return Format::Type::Float;
                 case spirv_cross::SPIRType::Int:
-                    return Format::Sint;
+                    return Format::Type::Sint;
                 case spirv_cross::SPIRType::UInt:
-                    return Format::Uint;
+                    return Format::Type::Uint;
                 default:
                     UNREACHABLE();
-                    return Format::Other;
+                    return Format::Type::Other;
             }
         }
 
@@ -463,7 +463,7 @@ namespace dawn_native {
                 UNREACHABLE();
         }
 
-        mFragmentOutputFormatBaseTypes.fill(Format::Other);
+        mFragmentOutputFormatBaseTypes.fill(Format::Type::Other);
         if (GetDevice()->IsToggleEnabled(Toggle::UseSpvcParser)) {
             mSpvcContext.SetUseSpvcParser(true);
         }

src/dawn_native/Texture.h

@@ -15,6 +15,8 @@
 #ifndef DAWNNATIVE_TEXTURE_H_
 #define DAWNNATIVE_TEXTURE_H_
 
+#include "common/ityp_bitset.h"
+#include "dawn_native/EnumClassBitmasks.h"
 #include "dawn_native/Error.h"
 #include "dawn_native/Forward.h"
 #include "dawn_native/ObjectBase.h"
@@ -24,6 +26,31 @@
 #include <vector>
 
 namespace dawn_native {
+
+    enum class Aspect : uint8_t {
+        Color = 0x1,
+        Depth = 0x2,
+        Stencil = 0x4,
+    };
+
+    template <>
+    struct EnumBitmaskSize<Aspect> {
+        static constexpr unsigned value = 3;
+    };
+
+}  // namespace dawn_native
+
+namespace wgpu {
+
+    template <>
+    struct IsDawnBitmask<dawn_native::Aspect> {
+        static constexpr bool enable = true;
+    };
+
+}  // namespace wgpu
+
+namespace dawn_native {
+
     MaybeError ValidateTextureDescriptor(const DeviceBase* device,
                                          const TextureDescriptor* descriptor);
     MaybeError ValidateTextureViewDescriptor(const TextureBase* texture,

src/dawn_native/metal/RenderPipelineMTL.mm

@@ -371,7 +371,7 @@ namespace dawn_native { namespace metal {
             descriptorMTL.colorAttachments[i].pixelFormat =
                 MetalPixelFormat(GetColorAttachmentFormat(i));
             const ColorStateDescriptor* descriptor = GetColorStateDescriptor(i);
-            bool isDeclaredInFragmentShader = fragmentOutputBaseTypes[i] != Format::Other;
+            bool isDeclaredInFragmentShader = fragmentOutputBaseTypes[i] != Format::Type::Other;
             ComputeBlendDesc(descriptorMTL.colorAttachments[i], descriptor,
                              isDeclaredInFragmentShader);
         }

src/dawn_native/opengl/CommandBufferGL.cpp

@@ -630,22 +630,16 @@ namespace dawn_native { namespace opengl {
                     gl.BindFramebuffer(GL_READ_FRAMEBUFFER, readFBO);
 
                     GLenum glAttachment = 0;
-                    switch (format.aspect) {
+                    if (format.aspects == (Aspect::Depth | Aspect::Stencil)) {
-                        case Format::Aspect::Color:
+                        glAttachment = GL_DEPTH_STENCIL_ATTACHMENT;
-                            glAttachment = GL_COLOR_ATTACHMENT0;
+                    } else if (format.aspects == Aspect::Depth) {
-                            break;
+                        glAttachment = GL_DEPTH_ATTACHMENT;
-                        case Format::Aspect::Depth:
+                    } else if (format.aspects == Aspect::Stencil) {
-                            glAttachment = GL_DEPTH_ATTACHMENT;
+                        glAttachment = GL_STENCIL_ATTACHMENT;
-                            break;
+                    } else if (format.aspects == Aspect::Color) {
-                        case Format::Aspect::Stencil:
+                        glAttachment = GL_COLOR_ATTACHMENT0;
-                            glAttachment = GL_STENCIL_ATTACHMENT;
+                    } else {
-                            break;
+                        UNREACHABLE();
-                        case Format::Aspect::DepthStencil:
-                            glAttachment = GL_DEPTH_STENCIL_ATTACHMENT;
-                            break;
-                        default:
-                            UNREACHABLE();
-                            break;
                     }
 
                     gl.BindBuffer(GL_PIXEL_PACK_BUFFER, buffer->GetHandle());
@@ -880,19 +874,14 @@ namespace dawn_native { namespace opengl {
                 GLenum glAttachment = 0;
                 // TODO(kainino@chromium.org): it may be valid to just always use
                 // GL_DEPTH_STENCIL_ATTACHMENT here.
-                switch (format.aspect) {
+                if (format.aspects == (Aspect::Depth | Aspect::Stencil)) {
-                    case Format::Aspect::Depth:
+                    glAttachment = GL_DEPTH_STENCIL_ATTACHMENT;
-                        glAttachment = GL_DEPTH_ATTACHMENT;
+                } else if (format.aspects == Aspect::Depth) {
-                        break;
+                    glAttachment = GL_DEPTH_ATTACHMENT;
-                    case Format::Aspect::Stencil:
+                } else if (format.aspects == Aspect::Stencil) {
-                        glAttachment = GL_STENCIL_ATTACHMENT;
+                    glAttachment = GL_STENCIL_ATTACHMENT;
-                        break;
+                } else {
-                    case Format::Aspect::DepthStencil:
+                    UNREACHABLE();
-                        glAttachment = GL_DEPTH_STENCIL_ATTACHMENT;
-                        break;
-                    default:
-                        UNREACHABLE();
-                        break;
                 }
 
                 if (textureView->GetTexture()->GetArrayLayers() == 1) {

src/dawn_native/vulkan/RenderPipelineVk.cpp

@@ -429,7 +429,7 @@ namespace dawn_native { namespace vulkan {
             descriptor->fragmentStage->module->GetFragmentOutputBaseTypes();
         for (uint32_t i : IterateBitSet(GetColorAttachmentsMask())) {
             const ColorStateDescriptor* colorStateDescriptor = GetColorStateDescriptor(i);
-            bool isDeclaredInFragmentShader = fragmentOutputBaseTypes[i] != Format::Other;
+            bool isDeclaredInFragmentShader = fragmentOutputBaseTypes[i] != Format::Type::Other;
             colorBlendAttachments[i] =
                 ComputeColorDesc(colorStateDescriptor, isDeclaredInFragmentShader);
         }

src/dawn_native/vulkan/TextureVk.cpp

@@ -17,6 +17,7 @@
 #include "common/Assert.h"
 #include "common/Math.h"
 #include "dawn_native/DynamicUploader.h"
+#include "dawn_native/EnumMaskIterator.h"
 #include "dawn_native/Error.h"
 #include "dawn_native/VulkanBackend.h"
 #include "dawn_native/vulkan/AdapterVk.h"
@@ -193,20 +194,25 @@ namespace dawn_native { namespace vulkan {
         }
 
         // Computes which Vulkan texture aspects are relevant for the given Dawn format
-        VkImageAspectFlags VulkanAspectMask(const Format& format) {
+        VkImageAspectFlags VulkanAspectMask(const Aspect& aspects) {
-            switch (format.aspect) {
+            VkImageAspectFlags flags = 0;
-                case Format::Aspect::Color:
+            for (Aspect aspect : IterateEnumMask(aspects)) {
-                    return VK_IMAGE_ASPECT_COLOR_BIT;
+                switch (aspect) {
-                case Format::Aspect::Depth:
+                    case Aspect::Color:
-                    return VK_IMAGE_ASPECT_DEPTH_BIT;
+                        flags |= VK_IMAGE_ASPECT_COLOR_BIT;
-                case Format::Aspect::Stencil:
+                        break;
-                    return VK_IMAGE_ASPECT_STENCIL_BIT;
+                    case Aspect::Depth:
-                case Format::Aspect::DepthStencil:
+                        flags |= VK_IMAGE_ASPECT_DEPTH_BIT;
-                    return VK_IMAGE_ASPECT_DEPTH_BIT | VK_IMAGE_ASPECT_STENCIL_BIT;
+                        break;
-                default:
+                    case Aspect::Stencil:
-                    UNREACHABLE();
+                        flags |= VK_IMAGE_ASPECT_STENCIL_BIT;
-                    return 0;
+                        break;
+                    default:
+                        UNREACHABLE();
+                        break;
+                }
             }
+            return flags;
         }
 
         VkImageMemoryBarrier BuildMemoryBarrier(const Format& format,
@@ -222,7 +228,7 @@ namespace dawn_native { namespace vulkan {
             barrier.oldLayout = VulkanImageLayout(lastUsage, format);
             barrier.newLayout = VulkanImageLayout(usage, format);
             barrier.image = image;
-            barrier.subresourceRange.aspectMask = VulkanAspectMask(format);
+            barrier.subresourceRange.aspectMask = VulkanAspectMask(format.aspects);
             barrier.subresourceRange.baseMipLevel = range.baseMipLevel;
             barrier.subresourceRange.levelCount = range.levelCount;
             barrier.subresourceRange.baseArrayLayer = range.baseArrayLayer;
@@ -664,7 +670,7 @@ namespace dawn_native { namespace vulkan {
     }
 
     VkImageAspectFlags Texture::GetVkAspectMask() const {
-        return VulkanAspectMask(GetFormat());
+        return VulkanAspectMask(GetFormat().aspects);
     }
 
     void Texture::TweakTransitionForExternalUsage(CommandRecordingContext* recordingContext,
@@ -1007,7 +1013,7 @@ namespace dawn_native { namespace vulkan {
         createInfo.format = VulkanImageFormat(device, descriptor->format);
         createInfo.components = VkComponentMapping{VK_COMPONENT_SWIZZLE_R, VK_COMPONENT_SWIZZLE_G,
                                                    VK_COMPONENT_SWIZZLE_B, VK_COMPONENT_SWIZZLE_A};
-        createInfo.subresourceRange.aspectMask = VulkanAspectMask(GetFormat());
+        createInfo.subresourceRange.aspectMask = VulkanAspectMask(GetFormat().aspects);
         createInfo.subresourceRange.baseMipLevel = descriptor->baseMipLevel;
         createInfo.subresourceRange.levelCount = descriptor->mipLevelCount;
         createInfo.subresourceRange.baseArrayLayer = descriptor->baseArrayLayer;

src/tests/BUILD.gn

@@ -156,6 +156,7 @@ test("dawn_unittests") {
     "unittests/BuddyMemoryAllocatorTests.cpp",
     "unittests/CommandAllocatorTests.cpp",
     "unittests/EnumClassBitmasksTests.cpp",
+    "unittests/EnumMaskIteratorTests.cpp",
     "unittests/ErrorTests.cpp",
     "unittests/ExtensionTests.cpp",
     "unittests/GetProcAddressTests.cpp",

src/tests/unittests/EnumMaskIteratorTests.cpp

@@ -0,0 +1,72 @@
+// 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.
+
+#include "dawn_native/EnumMaskIterator.h"
+
+#include "gtest/gtest.h"
+
+namespace dawn_native {
+
+    enum class TestAspect : uint8_t {
+        Color = 1,
+        Depth = 2,
+        Stencil = 4,
+    };
+
+    template <>
+    struct EnumBitmaskSize<TestAspect> {
+        static constexpr unsigned value = 3;
+    };
+
+}  // namespace dawn_native
+
+namespace wgpu {
+
+    template <>
+    struct IsDawnBitmask<dawn_native::TestAspect> {
+        static constexpr bool enable = true;
+    };
+
+}  // namespace wgpu
+
+namespace dawn_native {
+
+    static_assert(EnumBitmaskSize<TestAspect>::value == 3, "");
+
+    TEST(EnumMaskIteratorTests, None) {
+        for (TestAspect aspect : IterateEnumMask(static_cast<TestAspect>(0))) {
+            FAIL();
+            DAWN_UNUSED(aspect);
+        }
+    }
+
+    TEST(EnumMaskIteratorTests, All) {
+        TestAspect expected[] = {TestAspect::Color, TestAspect::Depth, TestAspect::Stencil};
+        uint32_t i = 0;
+        TestAspect aspects = TestAspect::Color | TestAspect::Depth | TestAspect::Stencil;
+        for (TestAspect aspect : IterateEnumMask(aspects)) {
+            EXPECT_EQ(aspect, expected[i++]);
+        }
+    }
+
+    TEST(EnumMaskIteratorTests, Partial) {
+        TestAspect expected[] = {TestAspect::Color, TestAspect::Stencil};
+        uint32_t i = 0;
+        TestAspect aspects = TestAspect::Stencil | TestAspect::Color;
+        for (TestAspect aspect : IterateEnumMask(aspects)) {
+            EXPECT_EQ(aspect, expected[i++]);
+        }
+    }
+
+}  // namespace dawn_native