From dd4584340da457c8fbd0f3c7472907cb5daa1f86 Mon Sep 17 00:00:00 2001 From: Ryan Harrison Date: Fri, 6 Dec 2019 14:41:49 +0000 Subject: [PATCH] Refactor D3D12 pipeline creation to better propagate errors BUG=dawn:301 Change-Id: Ia7982cfe40abb28ab786c8941e269bded11468ee Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/14282 Commit-Queue: Ryan Harrison Reviewed-by: Austin Eng --- .../d3d12/ComputePipelineD3D12.cpp | 17 ++++++++++++--- src/dawn_native/d3d12/ComputePipelineD3D12.h | 6 +++++- src/dawn_native/d3d12/DeviceD3D12.cpp | 2 +- src/dawn_native/d3d12/RenderPipelineD3D12.cpp | 3 ++- src/dawn_native/d3d12/RenderPipelineD3D12.h | 1 + src/dawn_native/d3d12/ShaderModuleD3D12.cpp | 21 ++++++++++++------- src/dawn_native/d3d12/ShaderModuleD3D12.h | 2 +- 7 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/dawn_native/d3d12/ComputePipelineD3D12.cpp b/src/dawn_native/d3d12/ComputePipelineD3D12.cpp index f1b94914a4..1fc81d635f 100644 --- a/src/dawn_native/d3d12/ComputePipelineD3D12.cpp +++ b/src/dawn_native/d3d12/ComputePipelineD3D12.cpp @@ -22,8 +22,17 @@ namespace dawn_native { namespace d3d12 { - ComputePipeline::ComputePipeline(Device* device, const ComputePipelineDescriptor* descriptor) - : ComputePipelineBase(device, descriptor) { + ResultOrError ComputePipeline::Create( + Device* device, + const ComputePipelineDescriptor* descriptor) { + std::unique_ptr pipeline = + std::make_unique(device, descriptor); + DAWN_TRY(pipeline->Initialize(descriptor)); + return pipeline.release(); + } + + MaybeError ComputePipeline::Initialize(const ComputePipelineDescriptor* descriptor) { + Device* device = ToBackend(GetDevice()); uint32_t compileFlags = 0; #if defined(_DEBUG) // Enable better shader debugging with the graphics debugging tools. @@ -33,7 +42,8 @@ namespace dawn_native { namespace d3d12 { compileFlags |= D3DCOMPILE_PACK_MATRIX_ROW_MAJOR; ShaderModule* module = ToBackend(descriptor->computeStage.module); - const std::string hlslSource = module->GetHLSLSource(ToBackend(GetLayout())); + std::string hlslSource; + DAWN_TRY_ASSIGN(hlslSource, module->GetHLSLSource(ToBackend(GetLayout()))); ComPtr compiledShader; ComPtr errors; @@ -53,6 +63,7 @@ namespace dawn_native { namespace d3d12 { device->GetD3D12Device()->CreateComputePipelineState(&d3dDesc, IID_PPV_ARGS(&mPipelineState)); + return {}; } ComputePipeline::~ComputePipeline() { diff --git a/src/dawn_native/d3d12/ComputePipelineD3D12.h b/src/dawn_native/d3d12/ComputePipelineD3D12.h index 7b1af9f670..9f38bbe8a4 100644 --- a/src/dawn_native/d3d12/ComputePipelineD3D12.h +++ b/src/dawn_native/d3d12/ComputePipelineD3D12.h @@ -25,12 +25,16 @@ namespace dawn_native { namespace d3d12 { class ComputePipeline : public ComputePipelineBase { public: - ComputePipeline(Device* device, const ComputePipelineDescriptor* descriptor); + static ResultOrError Create(Device* device, + const ComputePipelineDescriptor* descriptor); + ComputePipeline() = delete; ~ComputePipeline(); ComPtr GetPipelineState(); private: + using ComputePipelineBase::ComputePipelineBase; + MaybeError Initialize(const ComputePipelineDescriptor* descriptor); ComPtr mPipelineState; }; diff --git a/src/dawn_native/d3d12/DeviceD3D12.cpp b/src/dawn_native/d3d12/DeviceD3D12.cpp index 0e1ea4790b..204e55c90b 100644 --- a/src/dawn_native/d3d12/DeviceD3D12.cpp +++ b/src/dawn_native/d3d12/DeviceD3D12.cpp @@ -256,7 +256,7 @@ namespace dawn_native { namespace d3d12 { } ResultOrError Device::CreateComputePipelineImpl( const ComputePipelineDescriptor* descriptor) { - return new ComputePipeline(this, descriptor); + return ComputePipeline::Create(this, descriptor); } ResultOrError Device::CreatePipelineLayoutImpl( const PipelineLayoutDescriptor* descriptor) { diff --git a/src/dawn_native/d3d12/RenderPipelineD3D12.cpp b/src/dawn_native/d3d12/RenderPipelineD3D12.cpp index 89c9ed514c..830860be8e 100644 --- a/src/dawn_native/d3d12/RenderPipelineD3D12.cpp +++ b/src/dawn_native/d3d12/RenderPipelineD3D12.cpp @@ -337,7 +337,8 @@ namespace dawn_native { namespace d3d12 { break; } - const std::string hlslSource = module->GetHLSLSource(ToBackend(GetLayout())); + std::string hlslSource; + DAWN_TRY_ASSIGN(hlslSource, module->GetHLSLSource(ToBackend(GetLayout()))); const PlatformFunctions* functions = device->GetFunctions(); if (FAILED(functions->d3dCompile(hlslSource.c_str(), hlslSource.length(), nullptr, diff --git a/src/dawn_native/d3d12/RenderPipelineD3D12.h b/src/dawn_native/d3d12/RenderPipelineD3D12.h index affd5fe733..73408720c6 100644 --- a/src/dawn_native/d3d12/RenderPipelineD3D12.h +++ b/src/dawn_native/d3d12/RenderPipelineD3D12.h @@ -27,6 +27,7 @@ namespace dawn_native { namespace d3d12 { public: static ResultOrError Create(Device* device, const RenderPipelineDescriptor* descriptor); + RenderPipeline() = delete; ~RenderPipeline(); D3D12_PRIMITIVE_TOPOLOGY GetD3D12PrimitiveTopology() const; diff --git a/src/dawn_native/d3d12/ShaderModuleD3D12.cpp b/src/dawn_native/d3d12/ShaderModuleD3D12.cpp index c5425867a6..0e5c2ef18f 100644 --- a/src/dawn_native/d3d12/ShaderModuleD3D12.cpp +++ b/src/dawn_native/d3d12/ShaderModuleD3D12.cpp @@ -68,7 +68,7 @@ namespace dawn_native { namespace d3d12 { return {}; } - const std::string ShaderModule::GetHLSLSource(PipelineLayout* layout) { + ResultOrError ShaderModule::GetHLSLSource(PipelineLayout* layout) { std::unique_ptr compiler_impl; spirv_cross::CompilerHLSL* compiler; if (!GetDevice()->IsToggleEnabled(Toggle::UseSpvc)) { @@ -102,9 +102,13 @@ namespace dawn_native { namespace d3d12 { if (bindingInfo.used) { uint32_t bindingOffset = bindingOffsets[binding]; if (GetDevice()->IsToggleEnabled(Toggle::UseSpvc)) { - mSpvcContext.SetDecoration(bindingInfo.id, SHADERC_SPVC_DECORATION_BINDING, - bindingOffset); - // TODO(dawn:301): Check status & have some sort of meaningful error path + if (mSpvcContext.SetDecoration( + bindingInfo.id, SHADERC_SPVC_DECORATION_BINDING, bindingOffset) != + shaderc_spvc_status_success) { + return DAWN_VALIDATION_ERROR( + "Unable to set decorating binding before generating HLSL shader w/ " + "spvc"); + } } else { compiler->set_decoration(bindingInfo.id, spv::DecorationBinding, bindingOffset); @@ -114,9 +118,12 @@ namespace dawn_native { namespace d3d12 { } if (GetDevice()->IsToggleEnabled(Toggle::UseSpvc)) { shaderc_spvc::CompilationResult result; - mSpvcContext.CompileShader(&result); - // TODO(dawn:301): Check status & have some sort of meaningful error path - return result.GetStringOutput(); + if (mSpvcContext.CompileShader(&result) != shaderc_spvc_status_success) { + return DAWN_VALIDATION_ERROR("Unable to generate HLSL shader w/ spvc"); + } + std::string result_string = + result.GetStringOutput(); // Stripping const for ResultOrError + return result_string; } else { return compiler->compile(); } diff --git a/src/dawn_native/d3d12/ShaderModuleD3D12.h b/src/dawn_native/d3d12/ShaderModuleD3D12.h index bcec904779..0b4aeff19a 100644 --- a/src/dawn_native/d3d12/ShaderModuleD3D12.h +++ b/src/dawn_native/d3d12/ShaderModuleD3D12.h @@ -27,7 +27,7 @@ namespace dawn_native { namespace d3d12 { static ResultOrError Create(Device* device, const ShaderModuleDescriptor* descriptor); - const std::string GetHLSLSource(PipelineLayout* layout); + ResultOrError GetHLSLSource(PipelineLayout* layout); private: ShaderModule(Device* device, const ShaderModuleDescriptor* descriptor);