From fdc35e604e62b7ab152b6c97db3954ed286bb164 Mon Sep 17 00:00:00 2001 From: Zhaoming Jiang Date: Fri, 18 Jun 2021 13:29:39 +0000 Subject: [PATCH] Emit tint warning when creating shader module Add the formatted tint messages in OwnedCompilationMessages, which will be emit after creating the shader module succeeds of fails. This patch also change the compilation messages handling in creating shader modules. Now the compilation messages are separated from sparseResult, and should be handled manually. A new method, ShaderModuleBase::InjectCompilationMessages, is introduced to move a given OwnedCompilationMessages into the shader module, and emit the formatted tint errors and warnings. This method should be called explicitly on a valid or error shader module. Bug: dawn:753 Change-Id: I5825186c6d9c4aa7725aebd0c302bfce5e1f37cf Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/53890 Commit-Queue: Zhaoming Jiang Reviewed-by: Corentin Wallez --- src/dawn_native/CompilationMessages.cpp | 60 ++++++++++++++++-- src/dawn_native/CompilationMessages.h | 19 +++--- src/dawn_native/Device.cpp | 35 ++++++----- src/dawn_native/Device.h | 5 +- src/dawn_native/ShaderModule.cpp | 61 +++++++++++++------ src/dawn_native/ShaderModule.h | 21 +++---- src/dawn_native/vulkan/ShaderModuleVk.cpp | 12 ++-- .../ShaderModuleValidationTests.cpp | 6 +- 8 files changed, 150 insertions(+), 69 deletions(-) diff --git a/src/dawn_native/CompilationMessages.cpp b/src/dawn_native/CompilationMessages.cpp index 8e1b79cf5f..326cb9b9d2 100644 --- a/src/dawn_native/CompilationMessages.cpp +++ b/src/dawn_native/CompilationMessages.cpp @@ -41,12 +41,12 @@ namespace dawn_native { mCompilationInfo.messages = nullptr; } - void OwnedCompilationMessages::AddMessage(std::string message, - wgpu::CompilationMessageType type, - uint64_t lineNum, - uint64_t linePos, - uint64_t offset, - uint64_t length) { + void OwnedCompilationMessages::AddMessageForTesting(std::string message, + wgpu::CompilationMessageType type, + uint64_t lineNum, + uint64_t linePos, + uint64_t offset, + uint64_t length) { // Cannot add messages after GetCompilationInfo has been called. ASSERT(mCompilationInfo.messages == nullptr); @@ -111,6 +111,8 @@ namespace dawn_native { for (const auto& diag : diagnostics) { AddMessage(diag); } + + AddFormattedTintMessages(diagnostics); } void OwnedCompilationMessages::ClearMessages() { @@ -136,4 +138,50 @@ namespace dawn_native { return &mCompilationInfo; } + const std::vector& OwnedCompilationMessages::GetFormattedTintMessages() { + return mFormattedTintMessages; + } + + void OwnedCompilationMessages::AddFormattedTintMessages(const tint::diag::List& diagnostics) { + tint::diag::List messageList; + size_t warningCount = 0; + size_t errorCount = 0; + for (auto& diag : diagnostics) { + switch (diag.severity) { + case (tint::diag::Severity::Fatal): + case (tint::diag::Severity::Error): + case (tint::diag::Severity::InternalCompilerError): { + errorCount++; + messageList.add(tint::diag::Diagnostic(diag)); + break; + } + case (tint::diag::Severity::Warning): { + warningCount++; + messageList.add(tint::diag::Diagnostic(diag)); + break; + } + default: + break; + } + } + if (errorCount == 0 && warningCount == 0) { + return; + } + tint::diag::Formatter::Style style; + style.print_newline_at_end = false; + std::ostringstream t; + if (errorCount > 0) { + t << errorCount << " error(s) "; + if (warningCount > 0) { + t << "and "; + } + } + if (warningCount > 0) { + t << warningCount << " warning(s) "; + } + t << "generated while compiling the shader:" << std::endl + << tint::diag::Formatter{style}.format(messageList); + mFormattedTintMessages.push_back(t.str()); + } + } // namespace dawn_native diff --git a/src/dawn_native/CompilationMessages.h b/src/dawn_native/CompilationMessages.h index 8a681e6031..d6edbe9fa5 100644 --- a/src/dawn_native/CompilationMessages.h +++ b/src/dawn_native/CompilationMessages.h @@ -34,22 +34,27 @@ namespace dawn_native { OwnedCompilationMessages(); ~OwnedCompilationMessages() = default; - void AddMessage(std::string message, - wgpu::CompilationMessageType type = wgpu::CompilationMessageType::Info, - uint64_t lineNum = 0, - uint64_t linePos = 0, - uint64_t offset = 0, - uint64_t length = 0); - void AddMessage(const tint::diag::Diagnostic& diagnostic); + void AddMessageForTesting( + std::string message, + wgpu::CompilationMessageType type = wgpu::CompilationMessageType::Info, + uint64_t lineNum = 0, + uint64_t linePos = 0, + uint64_t offset = 0, + uint64_t length = 0); void AddMessages(const tint::diag::List& diagnostics); void ClearMessages(); const WGPUCompilationInfo* GetCompilationInfo(); + const std::vector& GetFormattedTintMessages(); private: + void AddMessage(const tint::diag::Diagnostic& diagnostic); + void AddFormattedTintMessages(const tint::diag::List& diagnostics); + WGPUCompilationInfo mCompilationInfo; std::vector mMessageStrings; std::vector mMessages; + std::vector mFormattedTintMessages; }; } // namespace dawn_native diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp index 84e79932cc..dd01d25a90 100644 --- a/src/dawn_native/Device.cpp +++ b/src/dawn_native/Device.cpp @@ -649,7 +649,8 @@ namespace dawn_native { ResultOrError> DeviceBase::GetOrCreateShaderModule( const ShaderModuleDescriptor* descriptor, - ShaderModuleParseResult* parseResult) { + ShaderModuleParseResult* parseResult, + OwnedCompilationMessages* compilationMessages) { ASSERT(parseResult != nullptr); ShaderModuleBase blueprint(this, descriptor); @@ -668,7 +669,8 @@ namespace dawn_native { // now, so call validate. Most of |ValidateShaderModuleDescriptor| is parsing, but // we can consider splitting it if additional validation is added. ASSERT(!IsValidationEnabled()); - DAWN_TRY(ValidateShaderModuleDescriptor(this, descriptor, parseResult)); + DAWN_TRY(ValidateShaderModuleDescriptor(this, descriptor, parseResult, + compilationMessages)); } DAWN_TRY_ASSIGN(result, CreateShaderModuleImpl(descriptor, parseResult)); result->SetIsCachedReference(); @@ -839,10 +841,15 @@ namespace dawn_native { } ShaderModuleBase* DeviceBase::APICreateShaderModule(const ShaderModuleDescriptor* descriptor) { Ref result; - ShaderModuleParseResult parseResult = {}; - if (ConsumedError(CreateShaderModule(descriptor, &parseResult), &result)) { - return ShaderModuleBase::MakeError(this, std::move(parseResult.compilationMessages)); + std::unique_ptr compilationMessages( + std::make_unique()); + if (ConsumedError(CreateShaderModule(descriptor, compilationMessages.get()), &result)) { + result = ShaderModuleBase::MakeError(this); } + // Move compilation messages into ShaderModuleBase and emit tint errors and warnings + // after all other operations are finished successfully. + result->InjectCompilationMessages(std::move(compilationMessages)); + return result.Detach(); } SwapChainBase* DeviceBase::APICreateSwapChain(Surface* surface, @@ -1228,22 +1235,20 @@ namespace dawn_native { ResultOrError> DeviceBase::CreateShaderModule( const ShaderModuleDescriptor* descriptor, - ShaderModuleParseResult* parseResult) { + OwnedCompilationMessages* compilationMessages) { DAWN_TRY(ValidateIsAlive()); - // ShaderModule can be called from inside dawn_native. If that's the case handle the error - // directly in Dawn and don't need the parse results since there should be no validation - // errors. - ShaderModuleParseResult ignoredResults; - if (parseResult == nullptr) { - parseResult = &ignoredResults; - } + // CreateShaderModule can be called from inside dawn_native. If that's the case handle the + // error directly in Dawn and no compilationMessages held in the shader module. It is ok as + // long as dawn_native don't use the compilationMessages of these internal shader modules. + ShaderModuleParseResult parseResult; if (IsValidationEnabled()) { - DAWN_TRY(ValidateShaderModuleDescriptor(this, descriptor, parseResult)); + DAWN_TRY(ValidateShaderModuleDescriptor(this, descriptor, &parseResult, + compilationMessages)); } - return GetOrCreateShaderModule(descriptor, parseResult); + return GetOrCreateShaderModule(descriptor, &parseResult, compilationMessages); } ResultOrError> DeviceBase::CreateSwapChain( diff --git a/src/dawn_native/Device.h b/src/dawn_native/Device.h index 1cb711afec..bd59142fea 100644 --- a/src/dawn_native/Device.h +++ b/src/dawn_native/Device.h @@ -133,7 +133,8 @@ namespace dawn_native { ResultOrError> GetOrCreateShaderModule( const ShaderModuleDescriptor* descriptor, - ShaderModuleParseResult* parseResult); + ShaderModuleParseResult* parseResult, + OwnedCompilationMessages* compilationMessages); void UncacheShaderModule(ShaderModuleBase* obj); Ref GetOrCreateAttachmentState(AttachmentStateBlueprint* blueprint); @@ -166,7 +167,7 @@ namespace dawn_native { ResultOrError> CreateSampler(const SamplerDescriptor* descriptor); ResultOrError> CreateShaderModule( const ShaderModuleDescriptor* descriptor, - ShaderModuleParseResult* parseResult = nullptr); + OwnedCompilationMessages* compilationMessages = nullptr); ResultOrError> CreateSwapChain(Surface* surface, const SwapChainDescriptor* descriptor); ResultOrError> CreateTexture(const TextureDescriptor* descriptor); diff --git a/src/dawn_native/ShaderModule.cpp b/src/dawn_native/ShaderModule.cpp index 7a5e14991b..0e18117ec2 100644 --- a/src/dawn_native/ShaderModule.cpp +++ b/src/dawn_native/ShaderModule.cpp @@ -1042,9 +1042,7 @@ namespace dawn_native { } } // anonymous namespace - ShaderModuleParseResult::ShaderModuleParseResult() - : compilationMessages(new OwnedCompilationMessages()) { - } + ShaderModuleParseResult::ShaderModuleParseResult() = default; ShaderModuleParseResult::~ShaderModuleParseResult() = default; ShaderModuleParseResult::ShaderModuleParseResult(ShaderModuleParseResult&& rhs) = default; @@ -1069,7 +1067,8 @@ namespace dawn_native { MaybeError ValidateShaderModuleDescriptor(DeviceBase* device, const ShaderModuleDescriptor* descriptor, - ShaderModuleParseResult* parseResult) { + ShaderModuleParseResult* parseResult, + OwnedCompilationMessages* outMessages) { ASSERT(parseResult != nullptr); const ChainedStruct* chainedDescriptor = descriptor->nextInChain; @@ -1080,8 +1079,6 @@ namespace dawn_native { DAWN_TRY(ValidateSingleSType(chainedDescriptor, wgpu::SType::ShaderModuleSPIRVDescriptor, wgpu::SType::ShaderModuleWGSLDescriptor)); - OwnedCompilationMessages* outMessages = parseResult->compilationMessages.get(); - ScopedTintICEHandler scopedICEHandler(device); const ShaderModuleSPIRVDescriptor* spirvDesc = nullptr; @@ -1236,13 +1233,8 @@ namespace dawn_native { } } - ShaderModuleBase::ShaderModuleBase( - DeviceBase* device, - ObjectBase::ErrorTag tag, - std::unique_ptr compilationMessages) - : CachedObject(device, tag), - mType(Type::Undefined), - mCompilationMessages(std::move(compilationMessages)) { + ShaderModuleBase::ShaderModuleBase(DeviceBase* device, ObjectBase::ErrorTag tag) + : CachedObject(device, tag), mType(Type::Undefined) { } ShaderModuleBase::~ShaderModuleBase() { @@ -1252,10 +1244,8 @@ namespace dawn_native { } // static - ShaderModuleBase* ShaderModuleBase::MakeError( - DeviceBase* device, - std::unique_ptr compilationMessages) { - return new ShaderModuleBase(device, ObjectBase::kError, std::move(compilationMessages)); + ShaderModuleBase* ShaderModuleBase::MakeError(DeviceBase* device) { + return new ShaderModuleBase(device, ObjectBase::kError); } bool ShaderModuleBase::HasEntryPoint(const std::string& entryPoint) const { @@ -1301,6 +1291,42 @@ namespace dawn_native { mCompilationMessages->GetCompilationInfo(), userdata); } + void ShaderModuleBase::InjectCompilationMessages( + std::unique_ptr compilationMessages) { + // TODO(zhaoming.jiang@intel.com): ensure the InjectCompilationMessages is properly + // handled for shader module returned from cache. + // InjectCompilationMessages should be called only once for a shader module, after + // it is created. However currently InjectCompilationMessages may be called on a + // shader module returned from cache rather than newly created, and violate the rule. + // We just skip the injection in this case for now, but a proper solution including + // ensure the cache goes before the validation is required. + if (mCompilationMessages != nullptr) { + return; + } + // Move the compilationMessages into the shader module and emit the tint errors and warnings + mCompilationMessages = std::move(compilationMessages); + + // Emit the formatted Tint errors and warnings within the moved compilationMessages + const std::vector& formattedTintMessages = + mCompilationMessages->GetFormattedTintMessages(); + if (formattedTintMessages.empty()) { + return; + } + std::ostringstream t; + for (auto pMessage = formattedTintMessages.begin(); pMessage != formattedTintMessages.end(); + pMessage++) { + if (pMessage != formattedTintMessages.begin()) { + t << std::endl; + } + t << *pMessage; + } + this->GetDevice()->EmitLog(WGPULoggingType_Warning, t.str().c_str()); + } + + OwnedCompilationMessages* ShaderModuleBase::GetCompilationMessages() const { + return mCompilationMessages.get(); + } + ResultOrError> ShaderModuleBase::GeneratePullingSpirv( const std::vector& spirv, const VertexState& vertexState, @@ -1358,7 +1384,6 @@ namespace dawn_native { mTintProgram = std::move(parseResult->tintProgram); mTintSource = std::move(parseResult->tintSource); mSpirv = std::move(parseResult->spirv); - mCompilationMessages = std::move(parseResult->compilationMessages); if (GetDevice()->IsToggleEnabled(Toggle::UseTintGenerator)) { DAWN_TRY_ASSIGN(mEntryPoints, ReflectShaderUsingTint(GetDevice(), mTintProgram.get())); diff --git a/src/dawn_native/ShaderModule.h b/src/dawn_native/ShaderModule.h index b5d949f10e..d39d464055 100644 --- a/src/dawn_native/ShaderModule.h +++ b/src/dawn_native/ShaderModule.h @@ -75,12 +75,12 @@ namespace dawn_native { std::unique_ptr tintProgram; std::unique_ptr tintSource; std::vector spirv; - std::unique_ptr compilationMessages; }; MaybeError ValidateShaderModuleDescriptor(DeviceBase* device, const ShaderModuleDescriptor* descriptor, - ShaderModuleParseResult* parseResult); + ShaderModuleParseResult* parseResult, + OwnedCompilationMessages* outMessages); MaybeError ValidateCompatibilityWithPipelineLayout(DeviceBase* device, const EntryPointMetadata& entryPoint, const PipelineLayoutBase* layout); @@ -142,9 +142,7 @@ namespace dawn_native { ShaderModuleBase(DeviceBase* device, const ShaderModuleDescriptor* descriptor); ~ShaderModuleBase() override; - static ShaderModuleBase* MakeError( - DeviceBase* device, - std::unique_ptr compilationMessages); + static ShaderModuleBase* MakeError(DeviceBase* device); // Return true iff the program has an entrypoint called `entryPoint`. bool HasEntryPoint(const std::string& entryPoint) const; @@ -165,6 +163,11 @@ namespace dawn_native { void APIGetCompilationInfo(wgpu::CompilationInfoCallback callback, void* userdata); + void InjectCompilationMessages( + std::unique_ptr compilationMessages); + + OwnedCompilationMessages* GetCompilationMessages() const; + ResultOrError> GeneratePullingSpirv( const std::vector& spirv, const VertexState& vertexState, @@ -177,10 +180,6 @@ namespace dawn_native { const std::string& entryPoint, BindGroupIndex pullingBufferBindingSet) const; - OwnedCompilationMessages* GetCompilationMessages() { - return mCompilationMessages.get(); - } - protected: MaybeError InitializeBase(ShaderModuleParseResult* parseResult); static ResultOrError ReflectShaderUsingSPIRVCross( @@ -188,9 +187,7 @@ namespace dawn_native { const std::vector& spirv); private: - ShaderModuleBase(DeviceBase* device, - ObjectBase::ErrorTag tag, - std::unique_ptr compilationMessages); + ShaderModuleBase(DeviceBase* device, ObjectBase::ErrorTag tag); // The original data in the descriptor for caching. enum class Type { Undefined, Spirv, Wgsl }; diff --git a/src/dawn_native/vulkan/ShaderModuleVk.cpp b/src/dawn_native/vulkan/ShaderModuleVk.cpp index e6875050b3..ea1a74f019 100644 --- a/src/dawn_native/vulkan/ShaderModuleVk.cpp +++ b/src/dawn_native/vulkan/ShaderModuleVk.cpp @@ -108,7 +108,8 @@ namespace dawn_native { namespace vulkan { tint::Program program; DAWN_TRY_ASSIGN(program, RunTransforms(&transformManager, parseResult->tintProgram.get(), - transformInputs, nullptr, GetCompilationMessages())); + transformInputs, nullptr, nullptr)); + // We will miss the messages generated in this RunTransforms. tint::writer::spirv::Generator generator(&program); if (!generator.Generate()) { @@ -119,12 +120,11 @@ namespace dawn_native { namespace vulkan { spirv = generator.result(); spirvPtr = &spirv; - ShaderModuleParseResult transformedParseResult; - transformedParseResult.tintProgram = - std::make_unique(std::move(program)); - transformedParseResult.spirv = spirv; + // Rather than use a new ParseResult object, we just reuse the original parseResult + parseResult->tintProgram = std::make_unique(std::move(program)); + parseResult->spirv = spirv; - DAWN_TRY(InitializeBase(&transformedParseResult)); + DAWN_TRY(InitializeBase(parseResult)); } else { DAWN_TRY(InitializeBase(parseResult)); spirvPtr = &GetSpirv(); diff --git a/src/tests/unittests/validation/ShaderModuleValidationTests.cpp b/src/tests/unittests/validation/ShaderModuleValidationTests.cpp index 1c68dd7ecb..69d4e0ae42 100644 --- a/src/tests/unittests/validation/ShaderModuleValidationTests.cpp +++ b/src/tests/unittests/validation/ShaderModuleValidationTests.cpp @@ -171,9 +171,9 @@ TEST_F(ShaderModuleValidationTest, GetCompilationMessages) { reinterpret_cast(shaderModule.Get()); dawn_native::OwnedCompilationMessages* messages = shaderModuleBase->GetCompilationMessages(); messages->ClearMessages(); - messages->AddMessage("Info Message"); - messages->AddMessage("Warning Message", wgpu::CompilationMessageType::Warning); - messages->AddMessage("Error Message", wgpu::CompilationMessageType::Error, 3, 4); + messages->AddMessageForTesting("Info Message"); + messages->AddMessageForTesting("Warning Message", wgpu::CompilationMessageType::Warning); + messages->AddMessageForTesting("Error Message", wgpu::CompilationMessageType::Error, 3, 4); auto callback = [](WGPUCompilationInfoRequestStatus status, const WGPUCompilationInfo* info, void* userdata) {