From 5931c0ac3bccc53e58c34b295c7296f8135f2792 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Thu, 15 Oct 2020 13:51:33 +0000 Subject: [PATCH] ShaderModule: Don't use Format::Type for reflection. Previously Format::Type was used instead of wgpu::TextureComponentType so that ::Other could server as a tag value. This was confusing and almost the single use of ::Other. In a follow-up CL the format baseType is changed to be a per-aspect value, and Format::Type. This CL is a self-contained step in that direction. Bug: dawn:527 Change-Id: Ida834087f45a8fca17670ffe8ebd4d5c4f1cd423 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/30102 Commit-Queue: Corentin Wallez Reviewed-by: Stephen White Reviewed-by: Austin Eng --- src/dawn_native/BindGroup.cpp | 4 +++- src/dawn_native/BindGroupLayout.cpp | 3 +-- src/dawn_native/BindingInfo.h | 2 +- src/dawn_native/PipelineLayout.cpp | 3 +-- src/dawn_native/RenderPipeline.cpp | 8 +++++--- src/dawn_native/ShaderModule.cpp | 17 ++++------------- src/dawn_native/ShaderModule.h | 11 ++++------- src/dawn_native/SpirvUtils.cpp | 9 +++++---- src/dawn_native/SpirvUtils.h | 3 ++- src/dawn_native/metal/RenderPipelineMTL.mm | 7 +++---- src/dawn_native/opengl/PipelineGL.cpp | 5 ++--- src/dawn_native/vulkan/RenderPipelineVk.cpp | 7 +++---- 12 files changed, 34 insertions(+), 45 deletions(-) diff --git a/src/dawn_native/BindGroup.cpp b/src/dawn_native/BindGroup.cpp index a2b79b4a46..9de7fd0e62 100644 --- a/src/dawn_native/BindGroup.cpp +++ b/src/dawn_native/BindGroup.cpp @@ -114,7 +114,9 @@ namespace dawn_native { switch (requiredUsage) { case wgpu::TextureUsage::Sampled: { - if (!texture->GetFormat().HasComponentType(bindingInfo.textureComponentType)) { + if (!texture->GetFormat().HasComponentType( + Format::TextureComponentTypeToFormatType( + bindingInfo.textureComponentType))) { return DAWN_VALIDATION_ERROR("texture component type usage mismatch"); } break; diff --git a/src/dawn_native/BindGroupLayout.cpp b/src/dawn_native/BindGroupLayout.cpp index 49159b018d..e59aa9d721 100644 --- a/src/dawn_native/BindGroupLayout.cpp +++ b/src/dawn_native/BindGroupLayout.cpp @@ -280,8 +280,7 @@ namespace dawn_native { mBindingInfo[i].binding = BindingNumber(binding.binding); mBindingInfo[i].type = binding.type; mBindingInfo[i].visibility = binding.visibility; - mBindingInfo[i].textureComponentType = - Format::TextureComponentTypeToFormatType(binding.textureComponentType); + mBindingInfo[i].textureComponentType = binding.textureComponentType; mBindingInfo[i].storageTextureFormat = binding.storageTextureFormat; mBindingInfo[i].minBufferBindingSize = binding.minBufferBindingSize; diff --git a/src/dawn_native/BindingInfo.h b/src/dawn_native/BindingInfo.h index 2699b2d75f..58247812d3 100644 --- a/src/dawn_native/BindingInfo.h +++ b/src/dawn_native/BindingInfo.h @@ -52,7 +52,7 @@ namespace dawn_native { BindingNumber binding; wgpu::ShaderStage visibility; wgpu::BindingType type; - Format::Type textureComponentType = Format::Type::Float; + wgpu::TextureComponentType textureComponentType = wgpu::TextureComponentType::Float; wgpu::TextureViewDimension viewDimension = wgpu::TextureViewDimension::Undefined; wgpu::TextureFormat storageTextureFormat = wgpu::TextureFormat::Undefined; bool hasDynamicOffset = false; diff --git a/src/dawn_native/PipelineLayout.cpp b/src/dawn_native/PipelineLayout.cpp index f81ff103d5..29981940ac 100644 --- a/src/dawn_native/PipelineLayout.cpp +++ b/src/dawn_native/PipelineLayout.cpp @@ -117,8 +117,7 @@ namespace dawn_native { entry.type = shaderBinding.type; entry.hasDynamicOffset = false; entry.viewDimension = shaderBinding.viewDimension; - entry.textureComponentType = - Format::FormatTypeToTextureComponentType(shaderBinding.textureComponentType); + entry.textureComponentType = shaderBinding.textureComponentType; entry.storageTextureFormat = shaderBinding.storageTextureFormat; entry.minBufferBindingSize = shaderBinding.minBufferBindingSize; return entry; diff --git a/src/dawn_native/RenderPipeline.cpp b/src/dawn_native/RenderPipeline.cpp index f4a2209514..8d267b6d71 100644 --- a/src/dawn_native/RenderPipeline.cpp +++ b/src/dawn_native/RenderPipeline.cpp @@ -144,7 +144,8 @@ namespace dawn_native { MaybeError ValidateColorStateDescriptor(const DeviceBase* device, const ColorStateDescriptor& descriptor, - Format::Type fragmentOutputBaseType) { + bool fragmentWritten, + wgpu::TextureComponentType fragmentOutputBaseType) { if (descriptor.nextInChain != nullptr) { return DAWN_VALIDATION_ERROR("nextInChain must be nullptr"); } @@ -161,8 +162,8 @@ namespace dawn_native { if (!format->IsColor() || !format->isRenderable) { return DAWN_VALIDATION_ERROR("Color format must be color renderable"); } - if (fragmentOutputBaseType != Format::Type::Other && - fragmentOutputBaseType != format->type) { + if (fragmentWritten && + fragmentOutputBaseType != Format::FormatTypeToTextureComponentType(format->type)) { return DAWN_VALIDATION_ERROR( "Color format must match the fragment stage output type"); } @@ -356,6 +357,7 @@ namespace dawn_native { i < ColorAttachmentIndex(static_cast(descriptor->colorStateCount)); ++i) { DAWN_TRY(ValidateColorStateDescriptor( device, descriptor->colorStates[static_cast(i)], + fragmentMetadata.fragmentOutputsWritten[i], fragmentMetadata.fragmentOutputFormatBaseTypes[i])); } diff --git a/src/dawn_native/ShaderModule.cpp b/src/dawn_native/ShaderModule.cpp index fe2aff7fdf..25c9407e4d 100644 --- a/src/dawn_native/ShaderModule.cpp +++ b/src/dawn_native/ShaderModule.cpp @@ -553,7 +553,7 @@ namespace dawn_native { info->viewDimension = SpirvDimToTextureViewDimension(imageType.dim, imageType.arrayed); info->textureComponentType = - SpirvBaseTypeToFormatType(textureComponentType); + SpirvBaseTypeToTextureComponentType(textureComponentType); if (imageType.ms) { info->type = wgpu::BindingType::MultisampledTexture; } else { @@ -682,12 +682,9 @@ namespace dawn_native { spirv_cross::SPIRType::BaseType shaderFragmentOutputBaseType = compiler.get_type(fragmentOutput.base_type_id).basetype; - Format::Type formatType = - SpirvBaseTypeToFormatType(shaderFragmentOutputBaseType); - if (formatType == Format::Type::Other) { - return DAWN_VALIDATION_ERROR("Unexpected Fragment output type"); - } - metadata->fragmentOutputFormatBaseTypes[attachment] = formatType; + metadata->fragmentOutputFormatBaseTypes[attachment] = + SpirvBaseTypeToTextureComponentType(shaderFragmentOutputBaseType); + metadata->fragmentOutputsWritten.set(attachment); } } @@ -771,12 +768,6 @@ namespace dawn_native { return {}; } - // EntryPointMetadata - - EntryPointMetadata::EntryPointMetadata() { - fragmentOutputFormatBaseTypes.fill(Format::Type::Other); - } - // ShaderModuleBase ShaderModuleBase::ShaderModuleBase(DeviceBase* device, const ShaderModuleDescriptor* descriptor) diff --git a/src/dawn_native/ShaderModule.h b/src/dawn_native/ShaderModule.h index 922b2bcc8c..5178412b15 100644 --- a/src/dawn_native/ShaderModule.h +++ b/src/dawn_native/ShaderModule.h @@ -52,8 +52,6 @@ namespace dawn_native { // pointers to EntryPointMetadata are safe to store as long as you also keep a Ref to the // ShaderModuleBase. struct EntryPointMetadata { - EntryPointMetadata(); - // Per-binding shader metadata contains some SPIRV specific information in addition to // most of the frontend per-binding information. struct ShaderBindingInfo : BindingInfo { @@ -76,11 +74,10 @@ namespace dawn_native { // The set of vertex attributes this entryPoint uses. std::bitset usedVertexAttributes; - // An array to record the basic types (float, int and uint) of the fragment shader outputs - // or Format::Type::Other means the fragment shader output is unused. - using FragmentOutputBaseTypes = - ityp::array; - FragmentOutputBaseTypes fragmentOutputFormatBaseTypes; + // An array to record the basic types (float, int and uint) of the fragment shader outputs. + ityp::array + fragmentOutputFormatBaseTypes; + ityp::bitset fragmentOutputsWritten; // The local workgroup size declared for a compute entry point (or 0s otehrwise). Origin3D localWorkgroupSize; diff --git a/src/dawn_native/SpirvUtils.cpp b/src/dawn_native/SpirvUtils.cpp index ecaf1726a0..511bade1cc 100644 --- a/src/dawn_native/SpirvUtils.cpp +++ b/src/dawn_native/SpirvUtils.cpp @@ -134,14 +134,15 @@ namespace dawn_native { } } - Format::Type SpirvBaseTypeToFormatType(spirv_cross::SPIRType::BaseType spirvBaseType) { + wgpu::TextureComponentType SpirvBaseTypeToTextureComponentType( + spirv_cross::SPIRType::BaseType spirvBaseType) { switch (spirvBaseType) { case spirv_cross::SPIRType::Float: - return Format::Type::Float; + return wgpu::TextureComponentType::Float; case spirv_cross::SPIRType::Int: - return Format::Type::Sint; + return wgpu::TextureComponentType::Sint; case spirv_cross::SPIRType::UInt: - return Format::Type::Uint; + return wgpu::TextureComponentType::Uint; default: UNREACHABLE(); } diff --git a/src/dawn_native/SpirvUtils.h b/src/dawn_native/SpirvUtils.h index ceb6fd6dcf..cd39c04985 100644 --- a/src/dawn_native/SpirvUtils.h +++ b/src/dawn_native/SpirvUtils.h @@ -37,7 +37,8 @@ namespace dawn_native { wgpu::TextureFormat SpirvImageFormatToTextureFormat(spv::ImageFormat format); // Returns the format "component type" corresponding to the SPIRV base type. - Format::Type SpirvBaseTypeToFormatType(spirv_cross::SPIRType::BaseType spirvBaseType); + wgpu::TextureComponentType SpirvBaseTypeToTextureComponentType( + spirv_cross::SPIRType::BaseType spirvBaseType); } // namespace dawn_native diff --git a/src/dawn_native/metal/RenderPipelineMTL.mm b/src/dawn_native/metal/RenderPipelineMTL.mm index 48e6f13d1e..3735e8954a 100644 --- a/src/dawn_native/metal/RenderPipelineMTL.mm +++ b/src/dawn_native/metal/RenderPipelineMTL.mm @@ -369,15 +369,14 @@ namespace dawn_native { namespace metal { } } - const EntryPointMetadata::FragmentOutputBaseTypes& fragmentOutputBaseTypes = - GetStage(SingleShaderStage::Fragment).metadata->fragmentOutputFormatBaseTypes; + const auto& fragmentOutputsWritten = + GetStage(SingleShaderStage::Fragment).metadata->fragmentOutputsWritten; for (ColorAttachmentIndex i : IterateBitSet(GetColorAttachmentsMask())) { descriptorMTL.colorAttachments[static_cast(i)].pixelFormat = MetalPixelFormat(GetColorAttachmentFormat(i)); const ColorStateDescriptor* descriptor = GetColorStateDescriptor(i); - bool isDeclaredInFragmentShader = fragmentOutputBaseTypes[i] != Format::Type::Other; ComputeBlendDesc(descriptorMTL.colorAttachments[static_cast(i)], descriptor, - isDeclaredInFragmentShader); + fragmentOutputsWritten[i]); } descriptorMTL.inputPrimitiveTopology = MTLInputPrimitiveTopology(GetPrimitiveTopology()); diff --git a/src/dawn_native/opengl/PipelineGL.cpp b/src/dawn_native/opengl/PipelineGL.cpp index 331cf5f37f..fd6dac7837 100644 --- a/src/dawn_native/opengl/PipelineGL.cpp +++ b/src/dawn_native/opengl/PipelineGL.cpp @@ -193,9 +193,8 @@ namespace dawn_native { namespace opengl { GLuint textureIndex = indices[combined.textureLocation.group][bindingIndex]; mUnitsForTextures[textureIndex].push_back(textureUnit); - Format::Type componentType = - bgl->GetBindingInfo(bindingIndex).textureComponentType; - shouldUseFiltering = componentType == Format::Type::Float; + shouldUseFiltering = bgl->GetBindingInfo(bindingIndex).textureComponentType == + wgpu::TextureComponentType::Float; } { const BindGroupLayoutBase* bgl = diff --git a/src/dawn_native/vulkan/RenderPipelineVk.cpp b/src/dawn_native/vulkan/RenderPipelineVk.cpp index 0389d6da63..e50c34da63 100644 --- a/src/dawn_native/vulkan/RenderPipelineVk.cpp +++ b/src/dawn_native/vulkan/RenderPipelineVk.cpp @@ -408,13 +408,12 @@ namespace dawn_native { namespace vulkan { // pre-computed in the ColorState ityp::array colorBlendAttachments; - const EntryPointMetadata::FragmentOutputBaseTypes& fragmentOutputBaseTypes = - GetStage(SingleShaderStage::Fragment).metadata->fragmentOutputFormatBaseTypes; + const auto& fragmentOutputsWritten = + GetStage(SingleShaderStage::Fragment).metadata->fragmentOutputsWritten; for (ColorAttachmentIndex i : IterateBitSet(GetColorAttachmentsMask())) { const ColorStateDescriptor* colorStateDescriptor = GetColorStateDescriptor(i); - bool isDeclaredInFragmentShader = fragmentOutputBaseTypes[i] != Format::Type::Other; colorBlendAttachments[i] = - ComputeColorDesc(colorStateDescriptor, isDeclaredInFragmentShader); + ComputeColorDesc(colorStateDescriptor, fragmentOutputsWritten[i]); } VkPipelineColorBlendStateCreateInfo colorBlend; colorBlend.sType = VK_STRUCTURE_TYPE_PIPELINE_COLOR_BLEND_STATE_CREATE_INFO;