Remove unused SPIRV-Cross shader reflection bits.

The only part of mGLEntryPoints actually used is the bindings, so
replace ShaderModuleGL::mGLEntryPoints with mBindings.
This required extracting BindingInfoArray from EntryPointMetadata,
making it visible in the dawn_native namespace.
Remove all non-bindings-related reflection, and MSL-specific checks
and workarounds.

Bug: dawn:1076
Change-Id: I05657c0c89f5d8a2185e55f9ad7c8f81d89a8e60
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/62180
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Stephen White <senorblanco@chromium.org>

src/dawn_native/PipelineLayout.cpp

@@ -144,7 +144,7 @@ namespace dawn_native {
 
         // Does the trivial conversions from a ShaderBindingInfo to a BindGroupLayoutEntry
         auto ConvertMetadataToEntry =
-            [](const EntryPointMetadata::ShaderBindingInfo& shaderBinding,
+            [](const ShaderBindingInfo& shaderBinding,
                const ExternalTextureBindingLayout* externalTextureBindingEntry)
             -> BindGroupLayoutEntry {
             BindGroupLayoutEntry entry = {};
@@ -242,7 +242,7 @@ namespace dawn_native {
             for (BindGroupIndex group(0); group < metadata.bindings.size(); ++group) {
                 for (const auto& bindingIt : metadata.bindings[group]) {
                     BindingNumber bindingNumber = bindingIt.first;
-                    const EntryPointMetadata::ShaderBindingInfo& shaderBinding = bindingIt.second;
+                    const ShaderBindingInfo& shaderBinding = bindingIt.second;
 
                     // Create the BindGroupLayoutEntry
                     BindGroupLayoutEntry entry =

src/dawn_native/ShaderModule.cpp

@@ -431,8 +431,7 @@ namespace dawn_native {
             return std::move(program);
         }
 
-        std::vector<uint64_t> GetBindGroupMinBufferSizes(
+        std::vector<uint64_t> GetBindGroupMinBufferSizes(const BindingGroupInfoMap& shaderBindings,
-            const EntryPointMetadata::BindingGroupInfoMap& shaderBindings,
                                                          const BindGroupLayoutBase* layout) {
             std::vector<uint64_t> requiredBufferSizes(layout->GetUnverifiedBufferCount());
             uint32_t packedIdx = 0;
@@ -471,7 +470,7 @@ namespace dawn_native {
             // corresponding binding in the BindGroupLayout, if it exists.
             for (const auto& it : entryPoint.bindings[group]) {
                 BindingNumber bindingNumber = it.first;
-                const EntryPointMetadata::ShaderBindingInfo& shaderInfo = it.second;
+                const ShaderBindingInfo& shaderInfo = it.second;
 
                 const auto& bindingIt = layoutBindings.find(bindingNumber);
                 if (bindingIt == layoutBindings.end()) {
@@ -825,12 +824,12 @@ namespace dawn_native {
                     }
 
                     const auto& it = metadata->bindings[bindGroupIndex].emplace(
-                        bindingNumber, EntryPointMetadata::ShaderBindingInfo{});
+                        bindingNumber, ShaderBindingInfo{});
                     if (!it.second) {
                         return DAWN_VALIDATION_ERROR("Shader has duplicate bindings");
                     }
 
-                    EntryPointMetadata::ShaderBindingInfo* info = &it.first->second;
+                    ShaderBindingInfo* info = &it.first->second;
                     info->bindingType = TintResourceTypeToBindingInfoType(resource.resource_type);
 
                     switch (info->bindingType) {

src/dawn_native/ShaderModule.h

@@ -115,11 +115,6 @@ namespace dawn_native {
                                          BindGroupIndex pullingBufferBindingSet,
                                          tint::transform::DataMap* transformInputs);
 
-    // Contains all the reflection data for a valid (ShaderModule, entryPoint, stage). They are
-    // stored in the ShaderModuleBase and destroyed only when the shader program is destroyed so
-    // pointers to EntryPointMetadata are safe to store as long as you also keep a Ref to the
-    // ShaderModuleBase.
-    struct EntryPointMetadata {
     // Mirrors wgpu::SamplerBindingLayout but instead stores a single boolean
     // for isComparison instead of a wgpu::SamplerBindingType enum.
     struct ShaderSamplerBindingInfo {
@@ -150,10 +145,16 @@ namespace dawn_native {
         StorageTextureBindingLayout storageTexture;
     };
 
-        // bindings[G][B] is the reflection data for the binding defined with
-        // [[group=G, binding=B]] in WGSL / SPIRV.
     using BindingGroupInfoMap = std::map<BindingNumber, ShaderBindingInfo>;
     using BindingInfoArray = ityp::array<BindGroupIndex, BindingGroupInfoMap, kMaxBindGroups>;
+
+    // Contains all the reflection data for a valid (ShaderModule, entryPoint, stage). They are
+    // stored in the ShaderModuleBase and destroyed only when the shader program is destroyed so
+    // pointers to EntryPointMetadata are safe to store as long as you also keep a Ref to the
+    // ShaderModuleBase.
+    struct EntryPointMetadata {
+        // bindings[G][B] is the reflection data for the binding defined with
+        // [[group=G, binding=B]] in WGSL / SPIRV.
         BindingInfoArray bindings;
 
         struct SamplerTexturePair {

src/dawn_native/d3d12/ShaderModuleD3D12.cpp

@@ -195,8 +195,7 @@ namespace dawn_native { namespace d3d12 {
         BindingRemapper::BindingPoints bindingPoints;
         BindingRemapper::AccessControls accessControls;
 
-        const EntryPointMetadata::BindingInfoArray& moduleBindingInfo =
+        const BindingInfoArray& moduleBindingInfo = GetEntryPoint(entryPointName).bindings;
-            GetEntryPoint(entryPointName).bindings;
 
         // d3d12::BindGroupLayout packs the bindings per HLSL register-space.
         // We modify the Tint AST to make the "bindings" decoration match the

src/dawn_native/opengl/ShaderModuleGL.cpp

@@ -66,31 +66,19 @@ namespace dawn_native { namespace opengl {
         return o.str();
     }
 
-    ResultOrError<std::unique_ptr<EntryPointMetadata>> ExtractSpirvInfo(
+    ResultOrError<std::unique_ptr<BindingInfoArray>> ExtractSpirvInfo(
         const DeviceBase* device,
         const spirv_cross::Compiler& compiler,
         const std::string& entryPointName,
         SingleShaderStage stage) {
-        std::unique_ptr<EntryPointMetadata> metadata = std::make_unique<EntryPointMetadata>();
-        metadata->stage = stage;
-
         const auto& resources = compiler.get_shader_resources();
 
-        if (resources.push_constant_buffers.size() > 0) {
-            return DAWN_VALIDATION_ERROR("Push constants aren't supported.");
-        }
-
-        if (resources.sampled_images.size() > 0) {
-            return DAWN_VALIDATION_ERROR("Combined images and samplers aren't supported.");
-        }
-
         // Fill in bindingInfo with the SPIRV bindings
         auto ExtractResourcesBinding =
             [](const DeviceBase* device,
                const spirv_cross::SmallVector<spirv_cross::Resource>& resources,
                const spirv_cross::Compiler& compiler, BindingInfoType bindingType,
-               EntryPointMetadata::BindingInfoArray* metadataBindings,
+               BindingInfoArray* bindings, bool isStorageBuffer = false) -> MaybeError {
-               bool isStorageBuffer = false) -> MaybeError {
             for (const auto& resource : resources) {
                 if (!compiler.get_decoration_bitset(resource.id).get(spv::DecorationBinding)) {
                     return DAWN_VALIDATION_ERROR("No Binding decoration set for resource");
@@ -110,13 +98,13 @@ namespace dawn_native { namespace opengl {
                     return DAWN_VALIDATION_ERROR("Bind group index over limits in the SPIRV");
                 }
 
-                const auto& it = (*metadataBindings)[bindGroupIndex].emplace(
+                const auto& it =
-                    bindingNumber, EntryPointMetadata::ShaderBindingInfo{});
+                    (*bindings)[bindGroupIndex].emplace(bindingNumber, ShaderBindingInfo{});
                 if (!it.second) {
                     return DAWN_VALIDATION_ERROR("Shader has duplicate bindings");
                 }
 
-                EntryPointMetadata::ShaderBindingInfo* info = &it.first->second;
+                ShaderBindingInfo* info = &it.first->second;
                 info->id = resource.id;
                 info->base_type_id = resource.base_type_id;
                 info->bindingType = bindingType;
@@ -220,92 +208,21 @@ namespace dawn_native { namespace opengl {
             return {};
         };
 
+        std::unique_ptr<BindingInfoArray> resultBindings = std::make_unique<BindingInfoArray>();
+        BindingInfoArray* bindings = resultBindings.get();
         DAWN_TRY(ExtractResourcesBinding(device, resources.uniform_buffers, compiler,
-                                         BindingInfoType::Buffer, &metadata->bindings));
+                                         BindingInfoType::Buffer, bindings));
         DAWN_TRY(ExtractResourcesBinding(device, resources.separate_images, compiler,
-                                         BindingInfoType::Texture, &metadata->bindings));
+                                         BindingInfoType::Texture, bindings));
         DAWN_TRY(ExtractResourcesBinding(device, resources.separate_samplers, compiler,
-                                         BindingInfoType::Sampler, &metadata->bindings));
+                                         BindingInfoType::Sampler, bindings));
         DAWN_TRY(ExtractResourcesBinding(device, resources.storage_buffers, compiler,
-                                         BindingInfoType::Buffer, &metadata->bindings, true));
+                                         BindingInfoType::Buffer, bindings, true));
         // ReadonlyStorageTexture is used as a tag to do general storage texture handling.
         DAWN_TRY(ExtractResourcesBinding(device, resources.storage_images, compiler,
-                                         BindingInfoType::StorageTexture, &metadata->bindings));
+                                         BindingInfoType::StorageTexture, resultBindings.get()));
 
-        // Extract the vertex attributes
+        return {std::move(resultBindings)};
-        if (stage == SingleShaderStage::Vertex) {
-            for (const auto& attrib : resources.stage_inputs) {
-                if (!(compiler.get_decoration_bitset(attrib.id).get(spv::DecorationLocation))) {
-                    return DAWN_VALIDATION_ERROR(
-                        "Unable to find Location decoration for Vertex input");
-                }
-                uint32_t unsanitizedLocation =
-                    compiler.get_decoration(attrib.id, spv::DecorationLocation);
-
-                if (unsanitizedLocation >= kMaxVertexAttributes) {
-                    return DAWN_VALIDATION_ERROR("Attribute location over limits in the SPIRV");
-                }
-                VertexAttributeLocation location(static_cast<uint8_t>(unsanitizedLocation));
-
-                spirv_cross::SPIRType::BaseType inputBaseType =
-                    compiler.get_type(attrib.base_type_id).basetype;
-                metadata->vertexInputBaseTypes[location] =
-                    SpirvBaseTypeToVertexFormatBaseType(inputBaseType);
-                metadata->usedVertexInputs.set(location);
-            }
-
-            // Without a location qualifier on vertex outputs, spirv_cross::CompilerMSL gives
-            // them all the location 0, causing a compile error.
-            for (const auto& attrib : resources.stage_outputs) {
-                if (!compiler.get_decoration_bitset(attrib.id).get(spv::DecorationLocation)) {
-                    return DAWN_VALIDATION_ERROR("Need location qualifier on vertex output");
-                }
-            }
-        }
-
-        if (stage == SingleShaderStage::Fragment) {
-            // Without a location qualifier on vertex inputs, spirv_cross::CompilerMSL gives
-            // them all the location 0, causing a compile error.
-            for (const auto& attrib : resources.stage_inputs) {
-                if (!compiler.get_decoration_bitset(attrib.id).get(spv::DecorationLocation)) {
-                    return DAWN_VALIDATION_ERROR("Need location qualifier on fragment input");
-                }
-            }
-
-            for (const auto& fragmentOutput : resources.stage_outputs) {
-                if (!compiler.get_decoration_bitset(fragmentOutput.id)
-                         .get(spv::DecorationLocation)) {
-                    return DAWN_VALIDATION_ERROR(
-                        "Unable to find Location decoration for Fragment output");
-                }
-                uint32_t unsanitizedAttachment =
-                    compiler.get_decoration(fragmentOutput.id, spv::DecorationLocation);
-
-                if (unsanitizedAttachment >= kMaxColorAttachments) {
-                    return DAWN_VALIDATION_ERROR(
-                        "Fragment output index must be less than max number of color "
-                        "attachments");
-                }
-                ColorAttachmentIndex attachment(static_cast<uint8_t>(unsanitizedAttachment));
-
-                spirv_cross::SPIRType::BaseType shaderFragmentOutputBaseType =
-                    compiler.get_type(fragmentOutput.base_type_id).basetype;
-                // spriv path so temporarily always set to 4u to always pass validation
-                metadata->fragmentOutputVariables[attachment] = {
-                    SpirvBaseTypeToTextureComponentType(shaderFragmentOutputBaseType), 4u};
-                metadata->fragmentOutputsWritten.set(attachment);
-            }
-        }
-
-        if (stage == SingleShaderStage::Compute) {
-            const spirv_cross::SPIREntryPoint& spirEntryPoint =
-                compiler.get_entry_point(entryPointName, spv::ExecutionModelGLCompute);
-            metadata->localWorkgroupSize.x = spirEntryPoint.workgroup_size.x;
-            metadata->localWorkgroupSize.y = spirEntryPoint.workgroup_size.y;
-            metadata->localWorkgroupSize.z = spirEntryPoint.workgroup_size.z;
-        }
-
-        return {std::move(metadata)};
     }
 
     // static
@@ -322,10 +239,10 @@ namespace dawn_native { namespace opengl {
     }
 
     // static
-    ResultOrError<EntryPointMetadataTable> ShaderModule::ReflectShaderUsingSPIRVCross(
+    ResultOrError<BindingInfoArrayTable> ShaderModule::ReflectShaderUsingSPIRVCross(
         DeviceBase* device,
         const std::vector<uint32_t>& spirv) {
-        EntryPointMetadataTable result;
+        BindingInfoArrayTable result;
         spirv_cross::Compiler compiler(spirv);
         for (const spirv_cross::EntryPoint& entryPoint : compiler.get_entry_points_and_stages()) {
             ASSERT(result.count(entryPoint.name) == 0);
@@ -333,9 +250,9 @@ namespace dawn_native { namespace opengl {
             SingleShaderStage stage = ExecutionModelToShaderStage(entryPoint.execution_model);
             compiler.set_entry_point(entryPoint.name, entryPoint.execution_model);
 
-            std::unique_ptr<EntryPointMetadata> metadata;
+            std::unique_ptr<BindingInfoArray> bindings;
-            DAWN_TRY_ASSIGN(metadata, ExtractSpirvInfo(device, compiler, entryPoint.name, stage));
+            DAWN_TRY_ASSIGN(bindings, ExtractSpirvInfo(device, compiler, entryPoint.name, stage));
-            result[entryPoint.name] = std::move(metadata);
+            result[entryPoint.name] = std::move(bindings);
         }
         return std::move(result);
     }
@@ -355,7 +272,7 @@ namespace dawn_native { namespace opengl {
             return DAWN_VALIDATION_ERROR(errorStream.str().c_str());
         }
 
-        DAWN_TRY_ASSIGN(mGLEntryPoints, ReflectShaderUsingSPIRVCross(GetDevice(), result.spirv));
+        DAWN_TRY_ASSIGN(mGLBindings, ReflectShaderUsingSPIRVCross(GetDevice(), result.spirv));
 
         return {};
     }
@@ -445,8 +362,7 @@ namespace dawn_native { namespace opengl {
             compiler.set_name(combined.combined_id, info->GetName());
         }
 
-        const EntryPointMetadata::BindingInfoArray& bindingInfo =
+        const BindingInfoArray& bindingInfo = *(mGLBindings.at(entryPointName));
-            (*mGLEntryPoints.at(entryPointName)).bindings;
 
         // Change binding names to be "dawn_binding_<group>_<binding>".
         // Also unsets the SPIRV "Binding" decoration as it outputs "layout(binding=)" which

src/dawn_native/opengl/ShaderModuleGL.h

@@ -44,6 +44,9 @@ namespace dawn_native { namespace opengl {
 
     using CombinedSamplerInfo = std::vector<CombinedSampler>;
 
+    using BindingInfoArrayTable =
+        std::unordered_map<std::string, std::unique_ptr<BindingInfoArray>>;
+
     class ShaderModule final : public ShaderModuleBase {
       public:
         static ResultOrError<Ref<ShaderModule>> Create(Device* device,
@@ -60,11 +63,11 @@ namespace dawn_native { namespace opengl {
         ShaderModule(Device* device, const ShaderModuleDescriptor* descriptor);
         ~ShaderModule() override = default;
         MaybeError Initialize(ShaderModuleParseResult* parseResult);
-        static ResultOrError<EntryPointMetadataTable> ReflectShaderUsingSPIRVCross(
+        static ResultOrError<BindingInfoArrayTable> ReflectShaderUsingSPIRVCross(
             DeviceBase* device,
             const std::vector<uint32_t>& spirv);
 
-        EntryPointMetadataTable mGLEntryPoints;
+        BindingInfoArrayTable mGLBindings;
     };
 
 }}  // namespace dawn_native::opengl

src/dawn_native/vulkan/ShaderModuleVk.cpp

@@ -121,8 +121,7 @@ namespace dawn_native { namespace vulkan {
         BindingRemapper::BindingPoints bindingPoints;
         BindingRemapper::AccessControls accessControls;
 
-        const EntryPointMetadata::BindingInfoArray& moduleBindingInfo =
+        const BindingInfoArray& moduleBindingInfo = GetEntryPoint(entryPointName).bindings;
-            GetEntryPoint(entryPointName).bindings;
 
         for (BindGroupIndex group : IterateBitSet(layout->GetBindGroupLayoutsMask())) {
             const BindGroupLayout* bgl = ToBackend(layout->GetBindGroupLayout(group));