Dawn: Dump shader module source before checking cache if required This CL make shader module sources always get dumped if DumpShaders toggle enabled, regardless of in-memory or on-disk cache hit or not. Without this CL, shader sources are dumped only if both caches missed and ParseShaderModule get called. This CL also fix a typo. Bug: 42240459, 402772740 Change-Id: I23f87567e726d6055c08293d59b33ef67f549800 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/247674 Reviewed-by: Loko Kung <lokokung@google.com> Reviewed-by: Corentin Wallez <cwallez@chromium.org> Commit-Queue: Zhaoming Jiang <zhaoming.jiang@microsoft.com>
diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp index 9c32973..ee22450 100644 --- a/src/dawn/native/Device.cpp +++ b/src/dawn/native/Device.cpp
@@ -2151,7 +2151,7 @@ Branch<ShaderSourceSPIRV, DawnShaderModuleSPIRVOptionsDescriptor, ShaderModuleCompilationOptions>>())); - // Module type specific validation. + // Module type specific validation switch (moduleType) { case wgpu::SType::ShaderSourceSPIRV: { DAWN_INVALID_IF(!TINT_BUILD_SPV_READER || IsToggleEnabled(Toggle::DisallowSpirv), @@ -2169,6 +2169,11 @@ DAWN_UNREACHABLE(); } + // Dump shader source code if required. + if (IsToggleEnabled(Toggle::DumpShaders)) { + DumpShaderFromDescriptor(this, unpacked); + } + // Check the cache and do actual validation and parsing if cache missed. ShaderModuleBase blueprint(this, unpacked, internalExtensions, ApiObjectBase::kUntrackedByDevice);
diff --git a/src/dawn/native/ShaderModule.cpp b/src/dawn/native/ShaderModule.cpp index 5f54066..c22a863 100644 --- a/src/dawn/native/ShaderModule.cpp +++ b/src/dawn/native/ShaderModule.cpp
@@ -1370,17 +1370,38 @@ DAWN_ASSERT(HasError()); } +void DumpShaderFromDescriptor(LogEmitter* logEmitter, + const UnpackedPtr<ShaderModuleDescriptor>& shaderModuleDesc) { +#if TINT_BUILD_SPV_READER + if ([[maybe_unused]] const auto* spirvDesc = shaderModuleDesc.Get<ShaderSourceSPIRV>()) { + // Dump SPIR-V if enabled. +#ifdef DAWN_ENABLE_SPIRV_VALIDATION + DumpSpirv(logEmitter, spirvDesc->code, spirvDesc->codeSize); +#endif // DAWN_ENABLE_SPIRV_VALIDATION + return; + } +#else // TINT_BUILD_SPV_READER + // SPIR-V is not enabled, so the descriptor should not contain it. + DAWN_ASSERT(shaderModuleDesc.Get<ShaderSourceSPIRV>() == nullptr); +#endif // TINT_BUILD_SPV_READER + + // Dump WGSL. + const ShaderSourceWGSL* wgslDesc = shaderModuleDesc.Get<ShaderSourceWGSL>(); + DAWN_ASSERT(wgslDesc != nullptr); + std::ostringstream dumpedMsg; + dumpedMsg << "// Dumped WGSL:\n" << std::string_view(wgslDesc->code) << "\n"; + logEmitter->EmitLog(wgpu::LoggingType::Info, dumpedMsg.str().c_str()); +} + ResultOrError<ShaderModuleParseResult> ParseShaderModule(ShaderModuleParseRequest req) { ShaderModuleParseResult outputParseResult; const ShaderModuleParseDeviceInfo& deviceInfo = req.deviceInfo; - LogEmitter* logEmitter = req.logEmitter.UnsafeGetValue(); - bool dumpShaders = deviceInfo.toggles.Has(Toggle::DumpShaders); #if TINT_BUILD_SPV_READER // Handling SPIR-V if enabled. if (std::holds_alternative<ShaderModuleParseSpirvDescription>(req.shaderDescription)) { - // SpirV toggle should have been validated before chacking cache. + // SpirV toggle should have been validated before checking cache. DAWN_ASSERT(!deviceInfo.toggles.Has(Toggle::DisallowSpirv)); ShaderModuleParseSpirvDescription& spirvDesc = @@ -1389,7 +1410,7 @@ #ifdef DAWN_ENABLE_SPIRV_VALIDATION MaybeError validationResult = - ValidateSpirv(logEmitter, spirvCode.data(), spirvCode.size(), dumpShaders); + ValidateSpirv(req.logEmitter.UnsafeGetValue(), spirvCode.data(), spirvCode.size()); // If SpirV validation error occurs, store it into outputParseResult and return. if (validationResult.IsError()) { outputParseResult.SetValidationError(validationResult.AcquireError()); @@ -1410,18 +1431,11 @@ if (std::holds_alternative<ShaderModuleParseWGSLDescription>(req.shaderDescription)) { ShaderModuleParseWGSLDescription wgslDesc = std::get<ShaderModuleParseWGSLDescription>(req.shaderDescription); - const StringView& wgsl = wgslDesc.wgsl.UnsafeGetValue(); const std::vector<tint::wgsl::Extension>& internalExtensions = wgslDesc.internalExtensions.UnsafeGetValue(); - + const StringView& wgsl = wgslDesc.wgsl.UnsafeGetValue(); auto tintFile = std::make_unique<tint::Source::File>("", wgsl); - if (dumpShaders) { - std::ostringstream dumpedMsg; - dumpedMsg << "// Dumped WGSL:\n" << std::string_view(wgsl) << "\n"; - logEmitter->EmitLog(wgpu::LoggingType::Info, dumpedMsg.str().c_str()); - } - DAWN_TRY(ParseWGSL(std::move(tintFile), deviceInfo.wgslAllowedFeatures, internalExtensions, &outputParseResult)); }
diff --git a/src/dawn/native/ShaderModule.h b/src/dawn/native/ShaderModule.h index fe1ad33..4009fb1 100644 --- a/src/dawn/native/ShaderModule.h +++ b/src/dawn/native/ShaderModule.h
@@ -55,6 +55,7 @@ #include "dawn/native/Forward.h" #include "dawn/native/IntegerTypes.h" #include "dawn/native/Limits.h" +#include "dawn/native/LogEmitter.h" #include "dawn/native/ObjectBase.h" #include "dawn/native/PerStage.h" #include "dawn/native/Serializable.h" @@ -153,6 +154,9 @@ std::string name; }; +void DumpShaderFromDescriptor(LogEmitter* logEmitter, + const UnpackedPtr<ShaderModuleDescriptor>& shaderModuleDesc); + // Parse a shader module from a validated ShaderModuleDescriptor, and generate reflection // information if required. Validation errors generated during parsing are also made cacheable and // returned within ShaderModuleParseResult together with compilation messages, rather than as an
diff --git a/src/dawn/native/SpirvValidation.cpp b/src/dawn/native/SpirvValidation.cpp index 61aac75..3c2d2ce0 100644 --- a/src/dawn/native/SpirvValidation.cpp +++ b/src/dawn/native/SpirvValidation.cpp
@@ -29,15 +29,13 @@ #include <spirv-tools/libspirv.hpp> +#include <memory> #include <sstream> #include <string> namespace dawn::native { -MaybeError ValidateSpirv(LogEmitter* logEmitter, - const uint32_t* spirv, - size_t wordCount, - bool dumpSpirv) { +MaybeError ValidateSpirv(LogEmitter* logEmitter, const uint32_t* spirv, size_t wordCount) { spvtools::SpirvTools spirvTools(SPV_ENV_VULKAN_1_1); spirvTools.SetMessageConsumer([logEmitter](spv_message_level_t level, const char*, const spv_position_t& position, @@ -71,17 +69,9 @@ val_opts.SetFriendlyNames(false); const bool valid = spirvTools.Validate(spirv, wordCount, val_opts); - if (dumpSpirv || !valid) { - std::ostringstream dumpedMsg; - std::string disassembly; - if (spirvTools.Disassemble( - spirv, wordCount, &disassembly, - SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES | SPV_BINARY_TO_TEXT_OPTION_INDENT)) { - dumpedMsg << "/* Dumped generated SPIRV disassembly */\n" << disassembly; - } else { - dumpedMsg << "/* Failed to disassemble generated SPIRV */"; - } - logEmitter->EmitLog(wgpu::LoggingType::Info, dumpedMsg.str().c_str()); + // Dump the generated SPIRV if it is invalid. + if (!valid) { + DumpSpirv(logEmitter, spirv, wordCount, &spirvTools); } DAWN_INVALID_IF(!valid, "Produced invalid SPIRV. Please file a bug at https://crbug.com/tint."); @@ -89,4 +79,26 @@ return {}; } +void DumpSpirv(LogEmitter* logEmitter, + const uint32_t* spirv, + size_t wordCount, + spvtools::SpirvTools* spirvTools) { + std::unique_ptr<spvtools::SpirvTools> inplaceSpirvTools; + if (spirvTools == nullptr) { + inplaceSpirvTools = std::make_unique<spvtools::SpirvTools>(SPV_ENV_VULKAN_1_1); + spirvTools = inplaceSpirvTools.get(); + } + + std::ostringstream dumpedMsg; + std::string disassembly; + if (spirvTools->Disassemble( + spirv, wordCount, &disassembly, + SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES | SPV_BINARY_TO_TEXT_OPTION_INDENT)) { + dumpedMsg << "/* Dumped SPIRV disassembly */\n" << disassembly; + } else { + dumpedMsg << "/* Failed to disassemble SPIRV */"; + } + logEmitter->EmitLog(wgpu::LoggingType::Info, dumpedMsg.str().c_str()); +} + } // namespace dawn::native
diff --git a/src/dawn/native/SpirvValidation.h b/src/dawn/native/SpirvValidation.h index a4c3595..9cb703c 100644 --- a/src/dawn/native/SpirvValidation.h +++ b/src/dawn/native/SpirvValidation.h
@@ -31,12 +31,18 @@ #include "dawn/native/Error.h" #include "dawn/native/LogEmitter.h" +namespace spvtools { +class SpirvTools; +} + namespace dawn::native { -MaybeError ValidateSpirv(LogEmitter* logEmitter, - const uint32_t* spirv, - size_t wordCount, - bool dumpSpirv); +MaybeError ValidateSpirv(LogEmitter* logEmitter, const uint32_t* spirv, size_t wordCount); + +void DumpSpirv(LogEmitter* logEmitter, + const uint32_t* spirv, + size_t wordCount, + spvtools::SpirvTools* spirvTools = nullptr); } // namespace dawn::native
diff --git a/src/dawn/native/vulkan/ShaderModuleVk.cpp b/src/dawn/native/vulkan/ShaderModuleVk.cpp index ba734ad..b2f44aa 100644 --- a/src/dawn/native/vulkan/ShaderModuleVk.cpp +++ b/src/dawn/native/vulkan/ShaderModuleVk.cpp
@@ -382,8 +382,11 @@ "Vulkan.CompileShaderToSPIRV"); #ifdef DAWN_ENABLE_SPIRV_VALIDATION - DAWN_TRY(ValidateSpirv(GetDevice(), compilation->spirv.data(), compilation->spirv.size(), - GetDevice()->IsToggleEnabled(Toggle::DumpShaders))); + // Validate and if required dump the compiled SPIR-V code. + DAWN_TRY(ValidateSpirv(GetDevice(), compilation->spirv.data(), compilation->spirv.size())); + if (GetDevice()->IsToggleEnabled(Toggle::DumpShaders)) { + DumpSpirv(GetDevice(), compilation->spirv.data(), compilation->spirv.size()); + } #endif VkShaderModuleCreateInfo createInfo;