From c686a6a54196b57e9c1843462632ff644433743d Mon Sep 17 00:00:00 2001 From: Jiawei Shao Date: Mon, 26 Jul 2021 01:39:21 +0000 Subject: [PATCH] Implement inter-stage variable matching rules - Part I This patch implements the inter-stage variable matching rules on the attributes 'location', 'base type' and 'composition type'. In the next patch we will implement the matching rules on the 'interpoliation type' and 'interpoliation sampling'. BUG=dawn:802 TEST=dawn_unittests Change-Id: Ic0a273e01dced301d437add83bad3d0c7d94a133 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/58363 Reviewed-by: Corentin Wallez Commit-Queue: Jiawei Shao --- src/common/Constants.h | 1 + src/dawn_native/RenderPipeline.cpp | 39 ++++++ src/dawn_native/ShaderModule.cpp | 49 ++++++- src/dawn_native/ShaderModule.h | 18 +++ src/tests/end2end/TextureViewTests.cpp | 3 +- .../RenderPipelineValidationTests.cpp | 121 ++++++++++++++++++ 6 files changed, 228 insertions(+), 3 deletions(-) diff --git a/src/common/Constants.h b/src/common/Constants.h index 998991f574..d6f42e71c2 100644 --- a/src/common/Constants.h +++ b/src/common/Constants.h @@ -25,6 +25,7 @@ static constexpr uint32_t kNumStages = 3; static constexpr uint8_t kMaxColorAttachments = 8u; static constexpr uint32_t kTextureBytesPerRowAlignment = 256u; static constexpr uint32_t kMaxInterStageShaderComponents = 60u; +static constexpr uint32_t kMaxInterStageShaderVariables = kMaxInterStageShaderComponents / 4; // Compute constants static constexpr uint32_t kMaxComputeWorkgroupStorageSize = 16352u; diff --git a/src/dawn_native/RenderPipeline.cpp b/src/dawn_native/RenderPipeline.cpp index cb9eac79fa..3831e95beb 100644 --- a/src/dawn_native/RenderPipeline.cpp +++ b/src/dawn_native/RenderPipeline.cpp @@ -23,6 +23,7 @@ #include "dawn_native/VertexFormat.h" #include +#include namespace dawn_native { // Helper functions @@ -311,6 +312,42 @@ namespace dawn_native { return {}; } + + MaybeError ValidateInterStageMatching(DeviceBase* device, + const VertexState& vertexState, + const FragmentState& fragmentState) { + const EntryPointMetadata& vertexMetadata = + vertexState.module->GetEntryPoint(vertexState.entryPoint); + const EntryPointMetadata& fragmentMetadata = + fragmentState.module->GetEntryPoint(fragmentState.entryPoint); + + if (vertexMetadata.usedInterStageVariables != + fragmentMetadata.usedInterStageVariables) { + return DAWN_VALIDATION_ERROR( + "One or more fragment inputs and vertex outputs are not one-to-one matching"); + } + + auto generateErrorString = [](const char* interStageAttribute, size_t location) { + std::ostringstream stream; + stream << "The " << interStageAttribute << " of the vertex output at location " + << location + << " is different from the one of the fragment input at the same location"; + return stream.str(); + }; + // TODO(dawn:802): Validate interpolation types and interpolition sampling types + for (size_t i : IterateBitSet(vertexMetadata.usedInterStageVariables)) { + const auto& vertexOutputInfo = vertexMetadata.interStageVariables[i]; + const auto& fragmentInputInfo = fragmentMetadata.interStageVariables[i]; + if (vertexOutputInfo.baseType != fragmentInputInfo.baseType) { + return DAWN_VALIDATION_ERROR(generateErrorString("base type", i)); + } + if (vertexOutputInfo.componentCount != fragmentInputInfo.componentCount) { + return DAWN_VALIDATION_ERROR(generateErrorString("componentCount", i)); + } + } + + return {}; + } } // anonymous namespace // Helper functions @@ -362,6 +399,8 @@ namespace dawn_native { return DAWN_VALIDATION_ERROR("Should have at least one color target or a depthStencil"); } + DAWN_TRY(ValidateInterStageMatching(device, descriptor->vertex, *(descriptor->fragment))); + return {}; } diff --git a/src/dawn_native/ShaderModule.cpp b/src/dawn_native/ShaderModule.cpp index 619a398cca..69ff2c14e8 100644 --- a/src/dawn_native/ShaderModule.cpp +++ b/src/dawn_native/ShaderModule.cpp @@ -337,6 +337,38 @@ namespace dawn_native { } } + ResultOrError TintComponentTypeToInterStageComponentType( + tint::inspector::ComponentType type) { + switch (type) { + case tint::inspector::ComponentType::kFloat: + return InterStageComponentType::Float; + case tint::inspector::ComponentType::kSInt: + return InterStageComponentType::Sint; + case tint::inspector::ComponentType::kUInt: + return InterStageComponentType::Uint; + case tint::inspector::ComponentType::kUnknown: + return DAWN_VALIDATION_ERROR( + "Attempted to convert 'Unknown' component type from Tint"); + } + } + + ResultOrError TintCompositionTypeToInterStageComponentCount( + tint::inspector::CompositionType type) { + switch (type) { + case tint::inspector::CompositionType::kScalar: + return 1u; + case tint::inspector::CompositionType::kVec2: + return 2u; + case tint::inspector::CompositionType::kVec3: + return 3u; + case tint::inspector::CompositionType::kVec4: + return 4u; + case tint::inspector::CompositionType::kUnknown: + return DAWN_VALIDATION_ERROR( + "Attempt to convert 'Unknown' composition type from Tint"); + } + } + MaybeError ValidateSpirv(const uint32_t* code, uint32_t codeSize) { spvtools::SpirvTools spirvTools(SPV_ENV_VULKAN_1_1); @@ -910,8 +942,7 @@ namespace dawn_native { return DAWN_VALIDATION_ERROR(errorStream.str().c_str()); } - constexpr uint32_t kMaxInterStageShaderLocation = - kMaxInterStageShaderComponents / 4 - 1; + constexpr uint32_t kMaxInterStageShaderLocation = kMaxInterStageShaderVariables - 1; for (auto& entryPoint : entryPoints) { ASSERT(result.count(entryPoint.name) == 0); @@ -1006,6 +1037,13 @@ namespace dawn_native { ss << "Vertex output location (" << location << ") over limits"; return DAWN_VALIDATION_ERROR(ss.str()); } + metadata->usedInterStageVariables.set(location); + DAWN_TRY_ASSIGN( + metadata->interStageVariables[location].baseType, + TintComponentTypeToInterStageComponentType(output_var.component_type)); + DAWN_TRY_ASSIGN(metadata->interStageVariables[location].componentCount, + TintCompositionTypeToInterStageComponentCount( + output_var.composition_type)); } } @@ -1021,6 +1059,13 @@ namespace dawn_native { ss << "Fragment input location (" << location << ") over limits"; return DAWN_VALIDATION_ERROR(ss.str()); } + metadata->usedInterStageVariables.set(location); + DAWN_TRY_ASSIGN( + metadata->interStageVariables[location].baseType, + TintComponentTypeToInterStageComponentType(input_var.component_type)); + DAWN_TRY_ASSIGN(metadata->interStageVariables[location].componentCount, + TintCompositionTypeToInterStageComponentCount( + input_var.composition_type)); } for (const auto& output_var : entryPoint.output_variables) { diff --git a/src/dawn_native/ShaderModule.h b/src/dawn_native/ShaderModule.h index da948e3900..2e501537f0 100644 --- a/src/dawn_native/ShaderModule.h +++ b/src/dawn_native/ShaderModule.h @@ -53,6 +53,13 @@ namespace dawn_native { struct EntryPointMetadata; + // Base component type of an inter-stage variable + enum class InterStageComponentType { + Sint, + Uint, + Float, + }; + using PipelineLayoutEntryPointPair = std::pair; struct PipelineLayoutEntryPointPairHashFunc { size_t operator()(const PipelineLayoutEntryPointPair& pair) const; @@ -157,6 +164,17 @@ namespace dawn_native { fragmentOutputFormatBaseTypes; ityp::bitset fragmentOutputsWritten; + // TODO(dawn:802): store InterpolationType and IntepolationSampling when we add the + // validations on them. + struct InterStageVariableInfo { + InterStageComponentType baseType; + uint32_t componentCount; + }; + // Now that we only support vertex and fragment stages, there can't be both inter-stage + // inputs and outputs in one shader stage. + std::bitset usedInterStageVariables; + std::array interStageVariables; + // The local workgroup size declared for a compute entry point (or 0s otehrwise). Origin3D localWorkgroupSize; diff --git a/src/tests/end2end/TextureViewTests.cpp b/src/tests/end2end/TextureViewTests.cpp index 61116d2fb3..afacd9483b 100644 --- a/src/tests/end2end/TextureViewTests.cpp +++ b/src/tests/end2end/TextureViewTests.cpp @@ -498,7 +498,8 @@ class TextureViewRenderingTest : public DawnTest { renderPassInfo.cColorAttachments[0].clearColor = {1.0f, 0.0f, 0.0f, 1.0f}; const char* oneColorFragmentShader = R"( - [[stage(fragment)]] fn main() -> [[location(0)]] vec4 { + [[stage(fragment)]] fn main([[location(0)]] texCoord : vec2) -> + [[location(0)]] vec4 { return vec4(0.0, 1.0, 0.0, 1.0); } )"; diff --git a/src/tests/unittests/validation/RenderPipelineValidationTests.cpp b/src/tests/unittests/validation/RenderPipelineValidationTests.cpp index bd78d8847e..62371e718c 100644 --- a/src/tests/unittests/validation/RenderPipelineValidationTests.cpp +++ b/src/tests/unittests/validation/RenderPipelineValidationTests.cpp @@ -832,3 +832,124 @@ TEST_F(DepthClampingValidationTest, Success) { device.CreateRenderPipeline(&descriptor); } } + +class InterStageVariableMatchingValidationTest : public RenderPipelineValidationTest { + protected: + void CheckCreatingRenderPipeline(wgpu::ShaderModule vertexModule, + wgpu::ShaderModule fragmentModule, + bool shouldSucceed) { + utils::ComboRenderPipelineDescriptor descriptor; + descriptor.vertex.module = vertexModule; + descriptor.cFragment.module = fragmentModule; + if (shouldSucceed) { + device.CreateRenderPipeline(&descriptor); + } else { + ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&descriptor)); + } + } +}; + +// Tests that creating render pipeline should fail when there is a vertex output that doesn't have +// its corresponding fragment input at the same location, and there is a fragment input that +// doesn't have its corresponding vertex output at the same location. +TEST_F(InterStageVariableMatchingValidationTest, MissingDeclarationAtSameLocation) { + wgpu::ShaderModule vertexModuleOutputAtLocation0 = utils::CreateShaderModule(device, R"( + struct A { + [[location(0)]] vout: f32; + [[builtin(position)]] pos: vec4; + }; + [[stage(vertex)]] fn main() -> A { + var vertexOut: A; + vertexOut.pos = vec4(0.0, 0.0, 0.0, 1.0); + return vertexOut; + })"); + wgpu::ShaderModule fragmentModuleAtLocation0 = utils::CreateShaderModule(device, R"( + struct B { + [[location(0)]] fin: f32; + }; + [[stage(fragment)]] fn main(fragmentIn: B) -> [[location(0)]] vec4 { + return vec4(fragmentIn.fin, 0.0, 0.0, 1.0); + })"); + wgpu::ShaderModule fragmentModuleInputAtLocation1 = utils::CreateShaderModule(device, R"( + struct A { + [[location(1)]] vout: f32; + }; + [[stage(fragment)]] fn main(vertexOut: A) -> [[location(0)]] vec4 { + return vec4(vertexOut.vout, 0.0, 0.0, 1.0); + })"); + wgpu::ShaderModule vertexModuleOutputAtLocation1 = utils::CreateShaderModule(device, R"( + struct B { + [[location(1)]] fin: f32; + [[builtin(position)]] pos: vec4; + }; + [[stage(vertex)]] fn main() -> B { + var fragmentIn: B; + fragmentIn.pos = vec4(0.0, 0.0, 0.0, 1.0); + return fragmentIn; + })"); + + { + CheckCreatingRenderPipeline(vertexModuleOutputAtLocation0, fsModule, false); + CheckCreatingRenderPipeline(vsModule, fragmentModuleAtLocation0, false); + CheckCreatingRenderPipeline(vertexModuleOutputAtLocation0, fragmentModuleInputAtLocation1, + false); + CheckCreatingRenderPipeline(vertexModuleOutputAtLocation1, fragmentModuleAtLocation0, + false); + } + + { + CheckCreatingRenderPipeline(vertexModuleOutputAtLocation0, fragmentModuleAtLocation0, true); + CheckCreatingRenderPipeline(vertexModuleOutputAtLocation1, fragmentModuleInputAtLocation1, + true); + } +} + +// Tests that creating render pipeline should fail when the type of a vertex stage output variable +// doesn't match the type of the fragment stage input variable at the same location. +TEST_F(InterStageVariableMatchingValidationTest, DifferentTypeAtSameLocation) { + constexpr std::array kTypes = {{"f32", "vec2", "vec3", "vec4", + "i32", "vec2", "vec3", "vec4", + "u32", "vec2", "vec3", "vec4"}}; + + std::array vertexModules; + std::array fragmentModules; + for (uint32_t i = 0; i < kTypes.size(); ++i) { + std::string interfaceDeclaration; + { + std::ostringstream sstream; + sstream << "struct A { [[location(0)]] a: " << kTypes[i] << ";" << std::endl; + interfaceDeclaration = sstream.str(); + } + { + std::ostringstream vertexStream; + vertexStream << interfaceDeclaration << R"( + [[builtin(position)]] pos: vec4; + }; + [[stage(vertex)]] fn main() -> A { + var vertexOut: A; + vertexOut.pos = vec4(0.0, 0.0, 0.0, 1.0); + return vertexOut; + })"; + vertexModules[i] = utils::CreateShaderModule(device, vertexStream.str().c_str()); + } + { + std::ostringstream fragmentStream; + fragmentStream << interfaceDeclaration << R"( + }; + [[stage(fragment)]] fn main(fragmentIn: A) -> [[location(0)]] vec4 { + return vec4(0.0, 0.0, 0.0, 1.0); + })"; + fragmentModules[i] = utils::CreateShaderModule(device, fragmentStream.str().c_str()); + } + } + + for (uint32_t vertexModuleIndex = 0; vertexModuleIndex < kTypes.size(); ++vertexModuleIndex) { + wgpu::ShaderModule vertexModule = vertexModules[vertexModuleIndex]; + for (uint32_t fragmentModuleIndex = 0; fragmentModuleIndex < kTypes.size(); + ++fragmentModuleIndex) { + wgpu::ShaderModule fragmentModule = fragmentModules[fragmentModuleIndex]; + bool shouldSuccess = vertexModuleIndex == fragmentModuleIndex; + CheckCreatingRenderPipeline(vertexModule, fragmentModule, shouldSuccess); + } + } +}