From 95e715873d61dba34a66d0979b99578c844a55d5 Mon Sep 17 00:00:00 2001 From: Ryan Harrison Date: Thu, 17 Sep 2020 19:58:40 +0000 Subject: [PATCH] Enable running robust buffer access pass on shaders In addition to adding the pass running logic, how shaders are stored in the ShaderModule is changed, so that cached shader that was used to create the ShaderModule is differentiated from the modified/intermediate shader that is actually being passed to SPIRV-Cross. Also rolling SPIRV-Tools to pick up a fix for a bug I discovered in the RBA pass implementation. Roll third_party/SPIRV-Tools/ fd05605be..34ef0c3fd (25 commits) https://chromium.googlesource.com/external/github.com/KhronosGroup/SPIRV-Tools/+log/fd05605bef1c..34ef0c3fdc8e $ git log fd05605be..34ef0c3fd --date=short --no-merges --format='%ad %ae %s' 2020-09-16 rharrison Fix missed modification flagging (#3814) 2020-09-16 andreperezmaselco.developer spirv-fuzz: Use an irrelevant id for the unused components (#3810) 2020-09-16 stefanomil spirv-fuzz: Improvements to random number generation (#3809) 2020-09-16 greg Add buffer oob check to bindless instrumentation (#3800) 2020-09-16 vasniktel spirv-fuzz: Remove CanFindOrCreateZeroConstant (#3807) 2020-09-15 andreperezmaselco.developer spirv-fuzz: Add bit instruction synonym transformation (#3775) 2020-09-16 vasniktel spirv-fuzz: Skip unreachable blocks (#3729) 2020-09-15 afdx Fix build errors (#3804) 2020-09-15 vasniktel spirv-fuzz: Handle invalid ids in fact manager (#3742) 2020-09-15 vasniktel spirv-fuzz: Support memory instructions MoveInstructionDown (#3700) 2020-09-15 stefanomil spirv-fuzz: Pass submanagers to other submanagers when necessary (#3796) 2020-09-15 stefanomil spirv-fuzz: Transformation to flatten conditional branch (#3667) 2020-09-14 46493288+sfricke-samsung spirv-val: Add BaseInstance, BaseVertex, DrawIndex, and ViewIndex (#3782) 2020-09-14 dnovillo Properly mark IR changed if instruction folder creates more than one constant. (#3799) 2020-09-11 afdx Add missing file to BUILD.gn (#3798) 2020-09-11 antonikarp spirv-fuzz: Add TransformationDuplicateRegionWithSelection (#3773) 2020-09-11 afdx spirv-reduce: Support reducing a specific function (#3774) 2020-09-10 afdx spirv-reduce: Refactoring (#3793) 2020-09-10 afdx Favour 'integrity' over 'coherence' as a replacement for 'sanity'. (#3619) 2020-09-10 antonikarp spirv-fuzz: Fix header guards in transformations/fuzzer passes (#3784) 2020-09-10 paulthomson spirv-fuzz: Add SPIRV_FUZZ_PROTOC_COMMAND (#3789) 2020-09-10 paulthomson Add missing include (#3788) 2020-09-09 paulthomson Improve spirv-fuzz CMake code (#3781) 2020-09-08 stevenperron Allow SPV_KHR_8bit_storage extension. (#3780) 2020-09-08 stefanomil spirv-opt: Add function to compute nesting depth of a block (#3771) Created with: roll-dep third_party/SPIRV-Tools BUG=dawn:523,dawn:480 Change-Id: I1f424f5fe6d67999412f286e831ea2ea26372b9e Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/28540 Reviewed-by: Kai Ninomiya Commit-Queue: Ryan Harrison --- DEPS | 2 +- src/dawn_native/BUILD.gn | 1 + src/dawn_native/ShaderModule.cpp | 73 +++++++++++++++++-- src/dawn_native/ShaderModule.h | 1 + .../end2end/VertexBufferRobustnessTests.cpp | 3 - 5 files changed, 70 insertions(+), 10 deletions(-) diff --git a/DEPS b/DEPS index a04cc80a68..634799bbde 100644 --- a/DEPS +++ b/DEPS @@ -72,7 +72,7 @@ deps = { # SPIRV compiler dependencies: SPIRV-Tools, SPIRV-headers, glslang and shaderc 'third_party/SPIRV-Tools': { - 'url': '{chromium_git}/external/github.com/KhronosGroup/SPIRV-Tools@fd05605bef1c63c6f249c19ed90c7ba24f8a8150', + 'url': '{chromium_git}/external/github.com/KhronosGroup/SPIRV-Tools@34ef0c3fdc8e235fd6217a64c824b6ceac93be60', 'condition': 'dawn_standalone', }, 'third_party/spirv-headers': { diff --git a/src/dawn_native/BUILD.gn b/src/dawn_native/BUILD.gn index 51a6761b47..23bc55a658 100644 --- a/src/dawn_native/BUILD.gn +++ b/src/dawn_native/BUILD.gn @@ -130,6 +130,7 @@ source_set("dawn_native_sources") { ":dawn_native_utils_gen", "${dawn_root}/src/common", "${dawn_root}/third_party/gn/spirv_cross:spirv_cross", + "${dawn_spirv_tools_dir}:spvtools_opt", "${dawn_spirv_tools_dir}:spvtools_val", ] defines = [] diff --git a/src/dawn_native/ShaderModule.cpp b/src/dawn_native/ShaderModule.cpp index 6b24b05cdf..65138262ec 100644 --- a/src/dawn_native/ShaderModule.cpp +++ b/src/dawn_native/ShaderModule.cpp @@ -22,6 +22,7 @@ #include "dawn_native/SpirvUtils.h" #include +#include #include #ifdef DAWN_ENABLE_WGSL @@ -327,6 +328,44 @@ namespace dawn_native { return requiredBufferSizes; } + ResultOrError> RunRobustBufferAccessPass( + const std::vector& spirv) { + spvtools::Optimizer opt(SPV_ENV_VULKAN_1_1); + + std::ostringstream errorStream; + errorStream << "SPIRV Optimizer failure:" << std::endl; + opt.SetMessageConsumer([&errorStream](spv_message_level_t level, const char*, + const spv_position_t& position, + const char* message) { + switch (level) { + case SPV_MSG_FATAL: + case SPV_MSG_INTERNAL_ERROR: + case SPV_MSG_ERROR: + errorStream << "error: line " << position.index << ": " << message + << std::endl; + break; + case SPV_MSG_WARNING: + errorStream << "warning: line " << position.index << ": " << message + << std::endl; + break; + case SPV_MSG_INFO: + errorStream << "info: line " << position.index << ": " << message + << std::endl; + break; + default: + break; + } + }); + opt.RegisterPass(spvtools::CreateGraphicsRobustAccessPass()); + + std::vector result; + if (!opt.Run(spirv.data(), spirv.size(), &result, spvtools::ValidatorOptions(), + false)) { + return DAWN_VALIDATION_ERROR(errorStream.str().c_str()); + } + return std::move(result); + } + MaybeError ValidateCompatibilityWithBindGroupLayout(BindGroupIndex group, const EntryPointMetadata& entryPoint, const BindGroupLayoutBase* layout) { @@ -741,7 +780,7 @@ namespace dawn_native { mType = Type::Spirv; const auto* spirvDesc = static_cast(descriptor->nextInChain); - mSpirv.assign(spirvDesc->code, spirvDesc->code + spirvDesc->codeSize); + mOriginalSpirv.assign(spirvDesc->code, spirvDesc->code + spirvDesc->codeSize); break; } case wgpu::SType::ShaderModuleWGSLDescriptor: { @@ -789,16 +828,23 @@ namespace dawn_native { size_t ShaderModuleBase::HashFunc::operator()(const ShaderModuleBase* module) const { size_t hash = 0; - for (uint32_t word : module->mSpirv) { + HashCombine(&hash, module->mType); + + for (uint32_t word : module->mOriginalSpirv) { HashCombine(&hash, word); } + for (char c : module->mWgsl) { + HashCombine(&hash, c); + } + return hash; } bool ShaderModuleBase::EqualityFunc::operator()(const ShaderModuleBase* a, const ShaderModuleBase* b) const { - return a->mSpirv == b->mSpirv; + return a->mType == b->mType && a->mOriginalSpirv == b->mOriginalSpirv && + a->mWgsl == b->mWgsl; } const std::vector& ShaderModuleBase::GetSpirv() const { @@ -810,20 +856,35 @@ namespace dawn_native { const VertexStateDescriptor& vertexState, const std::string& entryPoint, uint32_t pullingBufferBindingSet) const { - return ConvertWGSLToSPIRVWithPulling(mWgsl.c_str(), vertexState, entryPoint, - pullingBufferBindingSet); + std::vector spirv; + DAWN_TRY_ASSIGN(spirv, ConvertWGSLToSPIRVWithPulling(mWgsl.c_str(), vertexState, entryPoint, + pullingBufferBindingSet)); + if (GetDevice()->IsRobustnessEnabled()) { + DAWN_TRY_ASSIGN(spirv, RunRobustBufferAccessPass(spirv)); + } + + return std::move(spirv); } #endif MaybeError ShaderModuleBase::InitializeBase() { + std::vector spirv; if (mType == Type::Wgsl) { #ifdef DAWN_ENABLE_WGSL - DAWN_TRY_ASSIGN(mSpirv, ConvertWGSLToSPIRV(mWgsl.c_str())); + DAWN_TRY_ASSIGN(spirv, ConvertWGSLToSPIRV(mWgsl.c_str())); #else return DAWN_VALIDATION_ERROR("WGSL not supported (yet)"); #endif // DAWN_ENABLE_WGSL + } else { + spirv = mOriginalSpirv; } + if (GetDevice()->IsRobustnessEnabled()) { + DAWN_TRY_ASSIGN(spirv, RunRobustBufferAccessPass(spirv)); + } + + mSpirv = std::move(spirv); + spirv_cross::Compiler compiler(mSpirv); for (const spirv_cross::EntryPoint& entryPoint : compiler.get_entry_points_and_stages()) { SingleShaderStage stage = ExecutionModelToShaderStage(entryPoint.execution_model); diff --git a/src/dawn_native/ShaderModule.h b/src/dawn_native/ShaderModule.h index 6f9f2a3a1e..922b2bcc8c 100644 --- a/src/dawn_native/ShaderModule.h +++ b/src/dawn_native/ShaderModule.h @@ -129,6 +129,7 @@ namespace dawn_native { enum class Type { Undefined, Spirv, Wgsl }; Type mType; + std::vector mOriginalSpirv; std::vector mSpirv; std::string mWgsl; diff --git a/src/tests/end2end/VertexBufferRobustnessTests.cpp b/src/tests/end2end/VertexBufferRobustnessTests.cpp index 06052975da..b2ae2c4698 100644 --- a/src/tests/end2end/VertexBufferRobustnessTests.cpp +++ b/src/tests/end2end/VertexBufferRobustnessTests.cpp @@ -19,15 +19,12 @@ #include "utils/ComboRenderPipelineDescriptor.h" #include "utils/WGPUHelpers.h" -// TODO(rharrison): Re-enable as part of https://bugs.chromium.org/p/dawn/issues/detail?id=523 - // Vertex buffer robustness tests that clamping is applied on vertex attributes. This would happen // on backends where vertex pulling is enabled, such as Metal. class VertexBufferRobustnessTest : public DawnTest { protected: void SetUp() override { DawnTest::SetUp(); - GTEST_SKIP(); } // Creates a vertex module that tests an expression with given attributes. If successful, the