From 3a329f22096e76850160d7c794425e153164b86a Mon Sep 17 00:00:00 2001 From: Shrek Shao Date: Wed, 28 Sep 2022 18:36:47 +0000 Subject: [PATCH] Re-enable ValidationAfterOverrideStorageSize test Also move Robustness transform to pipeline creation time on Vulkan backend. Bug: tint:1660, dawn:1041 Change-Id: I10220f34119d11f29be86fd29463a282175eccdd Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/103780 Reviewed-by: Austin Eng Kokoro: Kokoro Commit-Queue: Shrek Shao --- src/dawn/native/null/DeviceNull.cpp | 22 +++++++++---------- src/dawn/native/vulkan/ShaderModuleVk.cpp | 19 +++++----------- .../tests/end2end/ShaderValidationTests.cpp | 7 +++--- 3 files changed, 20 insertions(+), 28 deletions(-) diff --git a/src/dawn/native/null/DeviceNull.cpp b/src/dawn/native/null/DeviceNull.cpp index badb570be5..df2106b62b 100644 --- a/src/dawn/native/null/DeviceNull.cpp +++ b/src/dawn/native/null/DeviceNull.cpp @@ -390,10 +390,12 @@ MaybeError ComputePipeline::Initialize() { tint::Program transformedProgram; const tint::Program* program; - if (!computeStage.metadata->overrides.empty()) { - tint::transform::Manager transformManager; - tint::transform::DataMap transformInputs; + tint::transform::Manager transformManager; + tint::transform::DataMap transformInputs; + transformManager.Add(); + + if (!computeStage.metadata->overrides.empty()) { transformManager.Add(); transformInputs.Add( computeStage.entryPoint.c_str()); @@ -403,16 +405,14 @@ MaybeError ComputePipeline::Initialize() { 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(); } + DAWN_TRY_ASSIGN(transformedProgram, + RunTransforms(&transformManager, computeStage.module->GetTintProgram(), + transformInputs, nullptr, nullptr)); + + program = &transformedProgram; + // Do the workgroup size validation as it is actually backend agnostic. const CombinedLimits& limits = GetDevice()->GetLimits(); Extent3D _; diff --git a/src/dawn/native/vulkan/ShaderModuleVk.cpp b/src/dawn/native/vulkan/ShaderModuleVk.cpp index 752338e904..b5796d6170 100644 --- a/src/dawn/native/vulkan/ShaderModuleVk.cpp +++ b/src/dawn/native/vulkan/ShaderModuleVk.cpp @@ -172,19 +172,7 @@ ShaderModule::ShaderModule(Device* device, const ShaderModuleDescriptor* descrip MaybeError ShaderModule::Initialize(ShaderModuleParseResult* parseResult, OwnedCompilationMessages* compilationMessages) { - if (GetDevice()->IsRobustnessEnabled()) { - ScopedTintICEHandler scopedICEHandler(GetDevice()); - - tint::transform::Robustness robustness; - tint::transform::DataMap transformInputs; - - tint::Program program; - DAWN_TRY_ASSIGN(program, RunTransforms(&robustness, parseResult->tintProgram.get(), - transformInputs, nullptr, nullptr)); - // Rather than use a new ParseResult object, we just reuse the original parseResult - parseResult->tintProgram = std::make_unique(std::move(program)); - } - + ScopedTintICEHandler scopedICEHandler(GetDevice()); return InitializeBase(parseResult, compilationMessages); } @@ -204,6 +192,7 @@ ShaderModule::~ShaderModule() = default; X(std::optional, substituteOverrideConfig) \ X(LimitsForCompilationRequest, limits) \ X(std::string_view, entryPointName) \ + X(bool, isRobustnessEnabled) \ X(bool, disableWorkgroupInit) \ X(bool, useZeroInitializeWorkgroupMemoryExtension) \ X(CacheKey::UnsafeUnkeyedValue, tracePlatform) @@ -284,6 +273,7 @@ ResultOrError ShaderModule::GetHandleAndSpirv( req.bindingPoints = std::move(bindingPoints); req.newBindingsMap = std::move(newBindingsMap); req.entryPointName = programmableStage.entryPoint; + req.isRobustnessEnabled = GetDevice()->IsRobustnessEnabled(); req.disableWorkgroupInit = GetDevice()->IsToggleEnabled(Toggle::DisableWorkgroupInit); req.useZeroInitializeWorkgroupMemoryExtension = GetDevice()->IsToggleEnabled(Toggle::VulkanUseZeroInitializeWorkgroupMemoryExtension); @@ -298,6 +288,9 @@ ResultOrError ShaderModule::GetHandleAndSpirv( spirv, GetDevice(), std::move(req), Spirv::FromBlob, [](SpirvCompilationRequest r) -> ResultOrError { tint::transform::Manager transformManager; + if (r.isRobustnessEnabled) { + transformManager.append(std::make_unique()); + } // Many Vulkan drivers can't handle multi-entrypoint shader modules. transformManager.append(std::make_unique()); // Run the binding remapper after SingleEntryPoint to avoid collisions with diff --git a/src/dawn/tests/end2end/ShaderValidationTests.cpp b/src/dawn/tests/end2end/ShaderValidationTests.cpp index 302a6baf64..9f964c9208 100644 --- a/src/dawn/tests/end2end/ShaderValidationTests.cpp +++ b/src/dawn/tests/end2end/ShaderValidationTests.cpp @@ -330,8 +330,7 @@ TEST_P(WorkgroupSizeValidationTest, ValidationAfterOverride) { // Test workgroup size validation after being overrided with invalid values (storage size limits // validation). -// TODO(tint:1660): re-enable after override can be used as array size. -TEST_P(WorkgroupSizeValidationTest, DISABLED_ValidationAfterOverrideStorageSize) { +TEST_P(WorkgroupSizeValidationTest, ValidationAfterOverrideStorageSize) { wgpu::Limits supportedLimits = GetSupportedLimits().limits; constexpr uint32_t kVec4Size = 16; @@ -347,11 +346,11 @@ TEST_P(WorkgroupSizeValidationTest, DISABLED_ValidationAfterOverrideStorageSize) ss << "override b: u32;"; if (vec4_count > 0) { ss << "var vec4_data: array, a>;"; - body << "_ = vec4_data;"; + body << "_ = vec4_data[0];"; } if (mat4_count > 0) { ss << "var mat4_data: array, b>;"; - body << "_ = mat4_data;"; + body << "_ = mat4_data[0];"; } ss << "@compute @workgroup_size(1) fn main() { " << body.str() << " }";