From 14579b9da3372438408c8b2c2c45a4779dc32893 Mon Sep 17 00:00:00 2001 From: shrekshao Date: Fri, 9 Sep 2022 00:49:08 +0000 Subject: [PATCH] Add Overrides implementation for OpenGL/OpenGLES backend As we are using SubstituteOverride, it is easy to add overridable constants support for OpenGL/OpenGLES. Also add validate workgroup size for null backend. Bug: dawn:1537, dawn:1504 Change-Id: I293f10b9a6c606aee6c0ed25b1d966bc56a0b88d Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/101800 Kokoro: Kokoro Reviewed-by: Austin Eng Commit-Queue: Shrek Shao --- src/dawn/native/d3d12/ShaderModuleD3D12.cpp | 4 +- src/dawn/native/metal/ShaderModuleMTL.mm | 4 +- src/dawn/native/null/DeviceNull.cpp | 37 ++++++++++++++++ src/dawn/native/opengl/ShaderModuleGL.cpp | 44 ++++++++++++++++--- src/dawn/native/vulkan/ShaderModuleVk.cpp | 4 +- src/dawn/tests/end2end/ShaderTests.cpp | 21 --------- .../tests/end2end/ShaderValidationTests.cpp | 8 +++- .../OverridableConstantsValidationTests.cpp | 5 +++ 8 files changed, 92 insertions(+), 35 deletions(-) diff --git a/src/dawn/native/d3d12/ShaderModuleD3D12.cpp b/src/dawn/native/d3d12/ShaderModuleD3D12.cpp index f67167e91d..a5392a1f7e 100644 --- a/src/dawn/native/d3d12/ShaderModuleD3D12.cpp +++ b/src/dawn/native/d3d12/ShaderModuleD3D12.cpp @@ -327,8 +327,8 @@ ResultOrError TranslateToHLSL( } if (r.substituteOverrideConfig) { - // This needs to run after SingleEntryPoint transform to get rid of overrides not used for - // the current entry point. + // This needs to run after SingleEntryPoint transform which removes unused overrides for + // current entry point. transformManager.Add(); transformInputs.Add( std::move(r.substituteOverrideConfig).value()); diff --git a/src/dawn/native/metal/ShaderModuleMTL.mm b/src/dawn/native/metal/ShaderModuleMTL.mm index b0f59a3b7e..e07cd1daa5 100644 --- a/src/dawn/native/metal/ShaderModuleMTL.mm +++ b/src/dawn/native/metal/ShaderModuleMTL.mm @@ -206,8 +206,8 @@ ResultOrError> TranslateToMSL( } if (r.substituteOverrideConfig) { - // This needs to run after SingleEntryPoint transform to get rid of overrides not - // used for the current entry point. + // This needs to run after SingleEntryPoint transform which removes unused overrides + // for current entry point. transformManager.Add(); transformInputs.Add( std::move(r.substituteOverrideConfig).value()); diff --git a/src/dawn/native/null/DeviceNull.cpp b/src/dawn/native/null/DeviceNull.cpp index 79d5f66e63..badb570be5 100644 --- a/src/dawn/native/null/DeviceNull.cpp +++ b/src/dawn/native/null/DeviceNull.cpp @@ -22,6 +22,9 @@ #include "dawn/native/ErrorData.h" #include "dawn/native/Instance.h" #include "dawn/native/Surface.h" +#include "dawn/native/TintUtils.h" + +#include "tint/tint.h" namespace dawn::native::null { @@ -383,6 +386,40 @@ MaybeError Queue::WriteBufferImpl(BufferBase* buffer, // ComputePipeline MaybeError ComputePipeline::Initialize() { + const ProgrammableStage& computeStage = GetStage(SingleShaderStage::Compute); + + tint::Program transformedProgram; + const tint::Program* program; + if (!computeStage.metadata->overrides.empty()) { + tint::transform::Manager transformManager; + tint::transform::DataMap transformInputs; + + transformManager.Add(); + transformInputs.Add( + computeStage.entryPoint.c_str()); + + // This needs to run after SingleEntryPoint transform which removes unused overrides for + // current entry point. + transformManager.Add(); + transformInputs.Add( + BuildSubstituteOverridesTransformConfig(computeStage)); + + DAWN_TRY_ASSIGN(transformedProgram, + RunTransforms(&transformManager, computeStage.module->GetTintProgram(), + transformInputs, nullptr, nullptr)); + + program = &transformedProgram; + } else { + program = computeStage.module->GetTintProgram(); + } + + // Do the workgroup size validation as it is actually backend agnostic. + const CombinedLimits& limits = GetDevice()->GetLimits(); + Extent3D _; + DAWN_TRY_ASSIGN( + _, ValidateComputeStageWorkgroupSize(*program, computeStage.entryPoint.c_str(), + LimitsForCompilationRequest::Create(limits.v1))); + return {}; } diff --git a/src/dawn/native/opengl/ShaderModuleGL.cpp b/src/dawn/native/opengl/ShaderModuleGL.cpp index 172d4015d9..caaf8634a4 100644 --- a/src/dawn/native/opengl/ShaderModuleGL.cpp +++ b/src/dawn/native/opengl/ShaderModuleGL.cpp @@ -57,13 +57,16 @@ tint::writer::glsl::Version::Standard ToTintGLStandard(opengl::OpenGLVersion::St using BindingMap = std::unordered_map; -#define GLSL_COMPILATION_REQUEST_MEMBERS(X) \ - X(const tint::Program*, inputProgram) \ - X(std::string, entryPointName) \ - X(tint::transform::MultiplanarExternalTexture::BindingsMap, externalTextureBindings) \ - X(BindingMap, glBindings) \ - X(opengl::OpenGLVersion::Standard, glVersionStandard) \ - X(uint32_t, glVersionMajor) \ +#define GLSL_COMPILATION_REQUEST_MEMBERS(X) \ + X(const tint::Program*, inputProgram) \ + X(std::string, entryPointName) \ + X(SingleShaderStage, stage) \ + X(tint::transform::MultiplanarExternalTexture::BindingsMap, externalTextureBindings) \ + X(BindingMap, glBindings) \ + X(std::optional, substituteOverrideConfig) \ + X(LimitsForCompilationRequest, limits) \ + X(opengl::OpenGLVersion::Standard, glVersionStandard) \ + X(uint32_t, glVersionMajor) \ X(uint32_t, glVersionMinor) DAWN_MAKE_CACHE_REQUEST(GLSLCompilationRequest, GLSL_COMPILATION_REQUEST_MEMBERS); @@ -168,11 +171,21 @@ ResultOrError ShaderModule::CompileShader(const OpenGLFunctions& gl, } } + std::optional substituteOverrideConfig; + if (!programmableStage.metadata->overrides.empty()) { + substituteOverrideConfig = BuildSubstituteOverridesTransformConfig(programmableStage); + } + + const CombinedLimits& limits = GetDevice()->GetLimits(); + GLSLCompilationRequest req = {}; req.inputProgram = GetTintProgram(); + req.stage = stage; req.entryPointName = programmableStage.entryPoint; req.externalTextureBindings = BuildExternalTextureTransformBindings(layout); req.glBindings = std::move(glBindings); + req.substituteOverrideConfig = std::move(substituteOverrideConfig); + req.limits = LimitsForCompilationRequest::Create(limits.v1); req.glVersionStandard = version.GetStandard(); req.glVersionMajor = version.GetMajor(); req.glVersionMinor = version.GetMinor(); @@ -190,10 +203,27 @@ ResultOrError ShaderModule::CompileShader(const OpenGLFunctions& gl, std::move(r.externalTextureBindings)); } + if (r.substituteOverrideConfig) { + transformManager.Add(); + transformInputs.Add(r.entryPointName); + // This needs to run after SingleEntryPoint transform which removes unused overrides + // for current entry point. + transformManager.Add(); + transformInputs.Add( + std::move(r.substituteOverrideConfig).value()); + } + tint::Program program; DAWN_TRY_ASSIGN(program, RunTransforms(&transformManager, r.inputProgram, transformInputs, nullptr, nullptr)); + if (r.stage == SingleShaderStage::Compute) { + // Validate workgroup size after program runs transforms. + Extent3D _; + DAWN_TRY_ASSIGN(_, ValidateComputeStageWorkgroupSize( + program, r.entryPointName.c_str(), r.limits)); + } + tint::writer::glsl::Options tintOptions; tintOptions.version = tint::writer::glsl::Version(ToTintGLStandard(r.glVersionStandard), r.glVersionMajor, r.glVersionMinor); diff --git a/src/dawn/native/vulkan/ShaderModuleVk.cpp b/src/dawn/native/vulkan/ShaderModuleVk.cpp index 6588627d96..0ec0092414 100644 --- a/src/dawn/native/vulkan/ShaderModuleVk.cpp +++ b/src/dawn/native/vulkan/ShaderModuleVk.cpp @@ -311,8 +311,8 @@ ResultOrError ShaderModule::GetHandleAndSpirv( r.newBindingsMap); } if (r.substituteOverrideConfig) { - // This needs to run after SingleEntryPoint transform to get rid of overrides not - // used for the current entry point. + // This needs to run after SingleEntryPoint transform which removes unused overrides + // for current entry point. transformManager.Add(); transformInputs.Add( std::move(r.substituteOverrideConfig).value()); diff --git a/src/dawn/tests/end2end/ShaderTests.cpp b/src/dawn/tests/end2end/ShaderTests.cpp index 45ba077516..3ce2759a20 100644 --- a/src/dawn/tests/end2end/ShaderTests.cpp +++ b/src/dawn/tests/end2end/ShaderTests.cpp @@ -395,9 +395,6 @@ fn main(@location(0) pos : vec4) -> @builtin(position) vec4 { // Test overridable constants without numeric identifiers TEST_P(ShaderTests, OverridableConstants) { - DAWN_TEST_UNSUPPORTED_IF(IsOpenGL()); - DAWN_TEST_UNSUPPORTED_IF(IsOpenGLES()); - uint32_t const kCount = 11; std::vector expected(kCount); std::iota(expected.begin(), expected.end(), 0); @@ -473,9 +470,6 @@ struct Buf { // Test one shader shared by two pipelines with different constants overridden TEST_P(ShaderTests, OverridableConstantsSharedShader) { - DAWN_TEST_UNSUPPORTED_IF(IsOpenGL()); - DAWN_TEST_UNSUPPORTED_IF(IsOpenGLES()); - std::vector expected1{1}; wgpu::Buffer buffer1 = CreateBuffer(expected1.size()); std::vector expected2{2}; @@ -530,9 +524,6 @@ struct Buf { // Test overridable constants work with workgroup size TEST_P(ShaderTests, OverridableConstantsWorkgroupSize) { - DAWN_TEST_UNSUPPORTED_IF(IsOpenGL()); - DAWN_TEST_UNSUPPORTED_IF(IsOpenGLES()); - std::string shader = R"( override x: u32; @@ -594,9 +585,6 @@ struct Buf { // Test overridable constants with numeric identifiers TEST_P(ShaderTests, OverridableConstantsNumericIdentifiers) { - DAWN_TEST_UNSUPPORTED_IF(IsOpenGL()); - DAWN_TEST_UNSUPPORTED_IF(IsOpenGLES()); - uint32_t const kCount = 4; std::vector expected{1u, 2u, 3u, 0u}; wgpu::Buffer buffer = CreateBuffer(kCount); @@ -651,9 +639,6 @@ struct Buf { // Test overridable constants precision // D3D12 HLSL shader uses defines so we want float number to have enough precision TEST_P(ShaderTests, OverridableConstantsPrecision) { - DAWN_TEST_UNSUPPORTED_IF(IsOpenGL()); - DAWN_TEST_UNSUPPORTED_IF(IsOpenGLES()); - uint32_t const kCount = 2; float const kValue1 = 3.14159; float const kValue2 = 3.141592653589793238; @@ -702,9 +687,6 @@ struct Buf { // Test overridable constants for different entry points TEST_P(ShaderTests, OverridableConstantsMultipleEntryPoints) { - DAWN_TEST_UNSUPPORTED_IF(IsOpenGL()); - DAWN_TEST_UNSUPPORTED_IF(IsOpenGLES()); - uint32_t const kCount = 1; std::vector expected1{1u}; std::vector expected2{2u}; @@ -807,9 +789,6 @@ struct Buf { // Draw a triangle covering the render target, with vertex position and color values from // overridable constants TEST_P(ShaderTests, OverridableConstantsRenderPipeline) { - DAWN_TEST_UNSUPPORTED_IF(IsOpenGL()); - DAWN_TEST_UNSUPPORTED_IF(IsOpenGLES()); - wgpu::ShaderModule vsModule = utils::CreateShaderModule(device, R"( @id(1111) override xright: f32; @id(2222) override ytop: f32; diff --git a/src/dawn/tests/end2end/ShaderValidationTests.cpp b/src/dawn/tests/end2end/ShaderValidationTests.cpp index c8b03f31db..e2e436553d 100644 --- a/src/dawn/tests/end2end/ShaderValidationTests.cpp +++ b/src/dawn/tests/end2end/ShaderValidationTests.cpp @@ -392,4 +392,10 @@ TEST_P(WorkgroupSizeValidationTest, DISABLED_ValidationAfterOverrideStorageSize) CheckPipelineWithWorkgroupStorage(false, 0, maxMat4Count + 1); } -DAWN_INSTANTIATE_TEST(WorkgroupSizeValidationTest, D3D12Backend(), MetalBackend(), VulkanBackend()); +DAWN_INSTANTIATE_TEST(WorkgroupSizeValidationTest, + D3D12Backend(), + MetalBackend(), + NullBackend(), + OpenGLBackend(), + OpenGLESBackend(), + VulkanBackend()); diff --git a/src/dawn/tests/unittests/validation/OverridableConstantsValidationTests.cpp b/src/dawn/tests/unittests/validation/OverridableConstantsValidationTests.cpp index 882bf25d24..c0f9eec3b5 100644 --- a/src/dawn/tests/unittests/validation/OverridableConstantsValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/OverridableConstantsValidationTests.cpp @@ -119,6 +119,11 @@ TEST_F(ComputePipelineOverridableConstantsValidationTest, ConstantsIdentifierLoo std::vector constants{{nullptr, "1000", 0}}; TestCreatePipeline(constants); } + { + // Error: c10 already has a constant numeric id specified + std::vector constants{{nullptr, "c10", 0}}; + ASSERT_DEVICE_ERROR(TestCreatePipeline(constants)); + } { // Error: constant numeric id not specified std::vector constants{{nullptr, "9999", 0}};