[tint] Plumb error handling through ShaderIO transform Adds error return paths through the ShaderIO transform infrastructure. This enables the GLSL backend to correctly identify and fail when BGRA swizzle is requested for a non-f32 value. Fixes: 499447283 Change-Id: I505c35e42e4f486ce9ea10e3ec14f75d060d84a6 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/303175 Reviewed-by: James Price <jrprice@google.com> Commit-Queue: James Price <jrprice@google.com> Auto-Submit: Ryan Harrison <rharrison@chromium.org> Commit-Queue: Ryan Harrison <rharrison@chromium.org>
diff --git a/src/tint/lang/core/ir/transform/shader_io.cc b/src/tint/lang/core/ir/transform/shader_io.cc index be9f89f..8f714c5 100644 --- a/src/tint/lang/core/ir/transform/shader_io.cc +++ b/src/tint/lang/core/ir/transform/shader_io.cc
@@ -61,7 +61,8 @@ std::unique_ptr<ShaderIOBackendState> backend{}; /// Process the module. - void Process() { + /// @returns Success if all of the sub-tasks succeed, otherwise propagates the failure reason + Result<SuccessType> Process() { // Collect all structures before the transform has run, so that we can strip their shader IO // attributes later. Vector<const type::Struct*, 16> structures_to_strip; @@ -80,7 +81,7 @@ continue; } - ProcessEntryPoint(func, make_backend_state(ir, func)); + TINT_CHECK_RESULT(ProcessEntryPoint(func, make_backend_state(ir, func))); } // Remove IO attributes from all structure members that had them prior to this transform. @@ -90,12 +91,15 @@ const_cast<core::type::StructMember*>(member)->ResetAttributes(); } } + + return Success; } /// Process an entry point. /// @param f the original entry point function /// @param bs the backend state object - void ProcessEntryPoint(Function* f, std::unique_ptr<ShaderIOBackendState> bs) { + /// @returns Success if all of the sub-tasks succeed, otherwise propagates the failure reason + Result<SuccessType> ProcessEntryPoint(Function* f, std::unique_ptr<ShaderIOBackendState> bs) { TINT_SCOPED_ASSIGNMENT(ep, f); backend = std::move(bs); TINT_DEFER(backend = nullptr); @@ -114,12 +118,12 @@ }); } - auto new_params = backend->FinalizeInputs(); - auto* new_ret_ty = backend->FinalizeOutputs(); + TINT_CHECK_RESULT_UNWRAP(new_params, backend->FinalizeInputs()); + TINT_CHECK_RESULT_UNWRAP(new_ret_ty, backend->FinalizeOutputs()); // Skip entry points with no new inputs or outputs. if (!backend->HasInputs() && !backend->HasOutputs()) { - return; + return Success; } // Rename the old function and remove its pipeline stage, workgroup size and subgroup size, @@ -157,6 +161,8 @@ // Return the new result. wrapper.Return(wrapper_ep, backend->MakeReturnValue(wrapper)); + + return Success; } /// Gather the shader inputs. @@ -334,8 +340,9 @@ return tint_global_invocation_index; } -void RunShaderIOBase(Module& module, std::function<MakeBackendStateFunc> make_backend_state) { - State{make_backend_state, module}.Process(); +Result<SuccessType> RunShaderIOBase(Module& module, + std::function<MakeBackendStateFunc> make_backend_state) { + return State{make_backend_state, module}.Process(); } ShaderIOBackendState::~ShaderIOBackendState() = default;
diff --git a/src/tint/lang/core/ir/transform/shader_io.h b/src/tint/lang/core/ir/transform/shader_io.h index 8d048bd..b568d26 100644 --- a/src/tint/lang/core/ir/transform/shader_io.h +++ b/src/tint/lang/core/ir/transform/shader_io.h
@@ -33,6 +33,7 @@ #include "src/tint/lang/core/ir/builder.h" #include "src/tint/lang/core/type/manager.h" +#include "src/tint/utils/result.h" namespace tint::core::ir::transform { @@ -73,12 +74,13 @@ bool HasOutputs() const { return !outputs.IsEmpty(); } /// Finalize the shader inputs and create any state needed for the new entry point function. - /// @returns the list of function parameters for the new entry point - virtual Vector<FunctionParam*, 4> FinalizeInputs() = 0; + /// @returns the list of function parameters for the new entry point on success, otherwise a + /// failure reason + virtual Result<Vector<FunctionParam*, 4>> FinalizeInputs() = 0; /// Finalize the shader outputs and create state needed for the new entry point function. - /// @returns the return type for the new entry point - virtual const type::Type* FinalizeOutputs() = 0; + /// @returns the return type for the new entry point on success, otherwise a failure reason + virtual Result<const type::Type*> FinalizeOutputs() = 0; /// Get the value of the input at index @p idx /// @param builder the IR builder for new instructions @@ -165,7 +167,9 @@ /// @param module the module to transform /// @param make_backend_state a function that creates a backend state object -void RunShaderIOBase(Module& module, std::function<MakeBackendStateFunc> make_backend_state); +/// @returns Success if all sub-tasks succeed, otherwise propagates the failure reason +Result<SuccessType> RunShaderIOBase(Module& module, + std::function<MakeBackendStateFunc> make_backend_state); } // namespace tint::core::ir::transform
diff --git a/src/tint/lang/glsl/writer/raise/shader_io.cc b/src/tint/lang/glsl/writer/raise/shader_io.cc index 7a3f66b..caddb31 100644 --- a/src/tint/lang/glsl/writer/raise/shader_io.cc +++ b/src/tint/lang/glsl/writer/raise/shader_io.cc
@@ -82,11 +82,12 @@ /// @param addrspace the address to use for the global variables /// @param access the access mode to use for the global variables /// @param name_suffix the suffix to add to struct and variable names - void MakeVars(Vector<core::ir::Var*, 4>& vars, - Vector<core::type::Manager::StructMemberDesc, 4>& entries, - core::AddressSpace addrspace, - core::Access access, - const char* name_suffix) { + /// @returns Success all operations succeed, otherwise returns a failure reason + Result<SuccessType> MakeVars(Vector<core::ir::Var*, 4>& vars, + Vector<core::type::Manager::StructMemberDesc, 4>& entries, + core::AddressSpace addrspace, + core::Access access, + const char* name_suffix) { for (auto io : entries) { StringStream name; name << ir.NameOf(func).Name(); @@ -146,6 +147,9 @@ // the components are reordered. if (addrspace == core::AddressSpace::kIn && config.bgra_swizzle_locations.count(*io.attributes.location) != 0) { + if (!io.type->DeepestElement()->Is<core::type::F32>()) { + return Failure{"BGRA swizzle is only supported for f32 types"}; + } bgra_swizzle_original_types.Add(*io.attributes.location, io.type); type = ty.vec4f(); } @@ -161,10 +165,11 @@ input_indices.Push(static_cast<uint32_t>(vars.Length())); vars.Push(var); } + return Success; } /// @copydoc ShaderIO::BackendState::FinalizeInputs - Vector<core::ir::FunctionParam*, 4> FinalizeInputs() override { + Result<Vector<core::ir::FunctionParam*, 4>> FinalizeInputs() override { // The following builtin values are polyfilled using other builtin values: // * workgroup_index - workgroup_id and num_workgroups // * global_invocation_index - global_invocation_id, num_workgroups (and workgroup size) @@ -185,13 +190,15 @@ "global_invocation_id"); } - MakeVars(input_vars, inputs, core::AddressSpace::kIn, core::Access::kRead, "_Input"); - return tint::Empty; + TINT_CHECK_RESULT( + MakeVars(input_vars, inputs, core::AddressSpace::kIn, core::Access::kRead, "_Input")); + return Vector<core::ir::FunctionParam*, 4>{}; } /// @copydoc ShaderIO::BackendState::FinalizeOutputs - const core::type::Type* FinalizeOutputs() override { - MakeVars(output_vars, outputs, core::AddressSpace::kOut, core::Access::kWrite, "_Output"); + Result<const core::type::Type*> FinalizeOutputs() override { + TINT_CHECK_RESULT(MakeVars(output_vars, outputs, core::AddressSpace::kOut, + core::Access::kWrite, "_Output")); return ty.void_(); } @@ -309,9 +316,10 @@ core::ir::Capability::kAllowDuplicateBindings}, "before glsl.ShaderIO"); - core::ir::transform::RunShaderIOBase(ir, [&](core::ir::Module& mod, core::ir::Function* func) { - return std::make_unique<StateImpl>(mod, func, config); - }); + TINT_CHECK_RESULT(core::ir::transform::RunShaderIOBase( + ir, [&](core::ir::Module& mod, core::ir::Function* func) { + return std::make_unique<StateImpl>(mod, func, config); + })); return Success; }
diff --git a/src/tint/lang/glsl/writer/raise/shader_io_test.cc b/src/tint/lang/glsl/writer/raise/shader_io_test.cc index 472338a..93aa10f 100644 --- a/src/tint/lang/glsl/writer/raise/shader_io_test.cc +++ b/src/tint/lang/glsl/writer/raise/shader_io_test.cc
@@ -70,6 +70,24 @@ EXPECT_EQ(expect, str()); } +TEST_F(GlslWriter_ShaderIOTest, BgraSwizzle_NonF32) { + auto* ep = b.Function("foo", ty.void_()); + auto* color = b.FunctionParam("color", ty.vec4i()); + color->SetLocation(0); + ep->SetParams({color}); + ep->SetStage(core::ir::Function::PipelineStage::kVertex); + + b.Append(ep->Block(), [&] { b.Return(ep); }); + + core::ir::transform::ImmediateDataLayout immediate_data; + ShaderIOConfig config{immediate_data}; + config.bgra_swizzle_locations.insert(0); + + auto result = RunWithFailure(ShaderIO, config); + EXPECT_NE(result, Success); + EXPECT_EQ(result.Failure().reason, "BGRA swizzle is only supported for f32 types"); +} + TEST_F(GlslWriter_ShaderIOTest, Parameters_NonStruct) { auto* ep = b.Function("foo", ty.void_()); auto* front_facing = b.FunctionParam("front_facing", ty.bool_());
diff --git a/src/tint/lang/hlsl/writer/raise/shader_io.cc b/src/tint/lang/hlsl/writer/raise/shader_io.cc index c8fc561..a0cb845 100644 --- a/src/tint/lang/hlsl/writer/raise/shader_io.cc +++ b/src/tint/lang/hlsl/writer/raise/shader_io.cc
@@ -230,7 +230,7 @@ } /// @copydoc ShaderIO::BackendState::FinalizeInputs - Vector<core::ir::FunctionParam*, 4> FinalizeInputs() override { + Result<Vector<core::ir::FunctionParam*, 4>> FinalizeInputs() override { if (config.add_input_position_member) { RequireBuiltinInput(core::BuiltinValue::kPosition, ty.vec4f(), "pos"); } @@ -359,14 +359,14 @@ TINT_IR_UNREACHABLE(ir); } input_param = b.FunctionParam("inputs", input_struct); - return {input_param}; + return Vector<core::ir::FunctionParam*, 4>{input_param}; } - return tint::Empty; + return Vector<core::ir::FunctionParam*, 4>{}; } /// @copydoc ShaderIO::BackendState::FinalizeOutputs - const core::type::Type* FinalizeOutputs() override { + Result<const core::type::Type*> FinalizeOutputs() override { if (outputs.IsEmpty()) { return ty.void_(); } @@ -739,9 +739,10 @@ core::ir::Capability::kAllow16BitIntegers}, "before hlsl.ShaderIO"); - core::ir::transform::RunShaderIOBase(ir, [&](core::ir::Module& mod, core::ir::Function* func) { - return std::make_unique<StateImpl>(mod, func, config); - }); + TINT_CHECK_RESULT(core::ir::transform::RunShaderIOBase( + ir, [&](core::ir::Module& mod, core::ir::Function* func) { + return std::make_unique<StateImpl>(mod, func, config); + })); return Success; }
diff --git a/src/tint/lang/msl/writer/raise/shader_io.cc b/src/tint/lang/msl/writer/raise/shader_io.cc index 88894e2..544377b 100644 --- a/src/tint/lang/msl/writer/raise/shader_io.cc +++ b/src/tint/lang/msl/writer/raise/shader_io.cc
@@ -85,7 +85,7 @@ ~StateImpl() override {} /// @copydoc ShaderIO::BackendState::FinalizeInputs - Vector<core::ir::FunctionParam*, 4> FinalizeInputs() override { + Result<Vector<core::ir::FunctionParam*, 4>> FinalizeInputs() override { // The following builtin values are polyfilled using other builtin values: // * workgroup_index - workgroup_id and num_workgroups // * global_invocation_index - global_invocation_id, num_workgroups (and workgroup size) @@ -179,7 +179,7 @@ } /// @copydoc ShaderIO::BackendState::FinalizeOutputs - const core::type::Type* FinalizeOutputs() override { + Result<const core::type::Type*> FinalizeOutputs() override { // Add a fixed sample mask builtin for fragment shaders if needed. if (config.fixed_sample_mask != UINT32_MAX && func->IsFragment()) { AddFixedSampleMaskOutput(); @@ -326,9 +326,10 @@ }, "before msl.ShaderIO"); - core::ir::transform::RunShaderIOBase(ir, [&](core::ir::Module& mod, core::ir::Function* func) { - return std::make_unique<StateImpl>(mod, func, config); - }); + TINT_CHECK_RESULT(core::ir::transform::RunShaderIOBase( + ir, [&](core::ir::Module& mod, core::ir::Function* func) { + return std::make_unique<StateImpl>(mod, func, config); + })); return Success; }
diff --git a/src/tint/lang/spirv/writer/raise/shader_io.cc b/src/tint/lang/spirv/writer/raise/shader_io.cc index 4703d03..d459a29 100644 --- a/src/tint/lang/spirv/writer/raise/shader_io.cc +++ b/src/tint/lang/spirv/writer/raise/shader_io.cc
@@ -248,7 +248,7 @@ } /// @copydoc ShaderIO::BackendState::FinalizeInputs - Vector<core::ir::FunctionParam*, 4> FinalizeInputs() override { + Result<Vector<core::ir::FunctionParam*, 4>> FinalizeInputs() override { if (config.multisampled_framebuffer_fetch) { sample_index_idx = RequireBuiltinInput(core::BuiltinValue::kSampleIndex, ty.u32(), "sample_idx"); @@ -275,11 +275,11 @@ } MakeVars(input_vars, inputs, core::AddressSpace::kIn, core::Access::kRead, "_Input"); - return tint::Empty; + return Vector<core::ir::FunctionParam*, 4>{}; } /// @copydoc ShaderIO::BackendState::FinalizeOutputs - const core::type::Type* FinalizeOutputs() override { + Result<const core::type::Type*> FinalizeOutputs() override { MakeVars(output_vars, outputs, core::AddressSpace::kOut, core::Access::kWrite, "_Output"); return ty.void_(); } @@ -474,9 +474,10 @@ Result<SuccessType> ShaderIO(core::ir::Module& ir, const ShaderIOConfig& config) { AssertValid(ir, kShaderIOCapabilities, "before spirv.ShaderIO"); - core::ir::transform::RunShaderIOBase(ir, [&](core::ir::Module& mod, core::ir::Function* func) { - return std::make_unique<StateImpl>(mod, func, config); - }); + TINT_CHECK_RESULT(core::ir::transform::RunShaderIOBase( + ir, [&](core::ir::Module& mod, core::ir::Function* func) { + return std::make_unique<StateImpl>(mod, func, config); + })); return Success; }