[spirv-reader][ir] Ignore `cull_distance`, skip unused builtins This CL adds the ability to skip the `cull_distance` annotated structure members when converting from SPIR-V. These get added as a standard part of the `gl_PerVertex` structure created when emitting GLSL as SPIR-V. This CL updates the ShaderIO routines to skip emitting unused members of builtin `kPointSize` or `kClipDistances` as they also get emitted even if unused. Any member which doesn't not have a location, builtin or or colour attribute attached are skipped. Bug: 42250952 Change-Id: Id6cd9cd9be45102ead9988ff53bf2932022fae31 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/220414 Reviewed-by: James Price <jrprice@google.com> Commit-Queue: dan sinclair <dsinclair@chromium.org>
diff --git a/src/tint/lang/core/ir/validator.cc b/src/tint/lang/core/ir/validator.cc index b5ab776..5d1a57a 100644 --- a/src/tint/lang/core/ir/validator.cc +++ b/src/tint/lang/core/ir/validator.cc
@@ -348,6 +348,17 @@ /* type_error */ "__point_size must be a f32", }; +constexpr BuiltinChecker kCullDistanceChecker{ + /* name */ "__cull_distance", + /* stages */ EnumSet<Function::PipelineStage>(Function::PipelineStage::kVertex), + /* direction */ BuiltinChecker::IODirection::kOutput, + /* type_check */ + [](const core::type::Type* ty) -> bool { + return ty->Is<core::type::Array>() && ty->DeepestElement()->Is<core::type::F32>(); + }, + /* type_error */ "__cull_distance must be an array of f32", +}; + constexpr BuiltinChecker kFragDepthChecker{ /* name */ "frag_depth", /* stages */ EnumSet<Function::PipelineStage>(Function::PipelineStage::kFragment), @@ -465,6 +476,8 @@ switch (builtin) { case BuiltinValue::kPointSize: return kPointSizeChecker; + case BuiltinValue::kCullDistance: + return kCullDistanceChecker; case BuiltinValue::kFragDepth: return kFragDepthChecker; case BuiltinValue::kFrontFacing:
diff --git a/src/tint/lang/spirv/reader/lower/shader_io.cc b/src/tint/lang/spirv/reader/lower/shader_io.cc index e1f375c..ddff8a4 100644 --- a/src/tint/lang/spirv/reader/lower/shader_io.cc +++ b/src/tint/lang/spirv/reader/lower/shader_io.cc
@@ -198,6 +198,10 @@ if (auto* str = var_type->As<core::type::Struct>()) { // Add an output for each member of the struct. for (auto* member : str->Members()) { + if (ShouldSkipMemberEmission(var, member)) { + continue; + } + // Use the base variable attributes if not specified directly on the member. auto member_attributes = member->Attributes(); if (auto base_loc = var_attributes.location) { @@ -253,6 +257,72 @@ } } + /// Returns true if the struct member should be skipped on emission + /// @param var the var which references the structure + /// @param member the member to check + /// @returns true if the member should be skipped. + bool ShouldSkipMemberEmission(core::ir::Var* var, const core::type::StructMember* member) { + auto var_attributes = var->Attributes(); + auto member_attributes = member->Attributes(); + + // If neither the var, nor the member has attributes, then skip + if (!var_attributes.builtin.has_value() && !var_attributes.color.has_value() && + !var_attributes.location.has_value()) { + if (!member_attributes.builtin.has_value() && !member_attributes.color.has_value() && + !member_attributes.location.has_value()) { + return true; + } + } + + // The `gl_PerVertex` structure always gets emitted by glslang, but it may only be used by + // the `gl_Position` variable. The structure will also contain the `gl_PointSize`, + // `gl_ClipDistance` and `gl_CullDistance`. + + if (member_attributes.builtin == core::BuiltinValue::kPointSize) { + // TODO(dsinclair): Validate that all accesses of this member are then used only to + // assign the value of 1.0. + return true; + } + if (member_attributes.builtin == core::BuiltinValue::kCullDistance) { + TINT_ASSERT(!IsIndexAccessed(var->Result(0), member->Index())); + return true; + } + if (member_attributes.builtin == core::BuiltinValue::kClipDistances) { + return !IsIndexAccessed(var->Result(0), member->Index()); + } + return false; + } + + /// Returns true if the `idx` member of `val` is accessed. The `val` must be of type + /// `Structure` which contains the given member index. + /// @param val the value to check + /// @param idx the index to look for + /// @returns true if `idx` is accessed. + bool IsIndexAccessed(core::ir::Value* val, uint32_t idx) { + for (auto& usage : val->UsagesUnsorted()) { + // Only care about access chains + auto* chain = usage->instruction->As<core::ir::Access>(); + if (!chain) { + continue; + } + if (chain->Indices().IsEmpty()) { + continue; + } + + // A member access has to be a constant index + auto* cnst = chain->Indices()[0]->As<core::ir::Constant>(); + if (!cnst) { + continue; + } + + uint32_t v = cnst->Value()->ValueAs<uint32_t>(); + if (v == idx) { + return true; + } + } + return false; + } + /// Replace a use of an input pointer value. /// @param var the originating input variable /// @param value the input pointer value
diff --git a/src/tint/lang/spirv/reader/lower/shader_io_test.cc b/src/tint/lang/spirv/reader/lower/shader_io_test.cc index a19b8e2..9b06a0b 100644 --- a/src/tint/lang/spirv/reader/lower/shader_io_test.cc +++ b/src/tint/lang/spirv/reader/lower/shader_io_test.cc
@@ -2144,5 +2144,266 @@ EXPECT_EQ(expect, str()); } +TEST_F(SpirvReader_ShaderIOTest, PointSize) { + auto* builtin_str = + ty.Struct(mod.symbols.New("Builtins"), Vector{ + core::type::Manager::StructMemberDesc{ + mod.symbols.New("position"), + ty.vec4<f32>(), + BuiltinAttrs(core::BuiltinValue::kPosition), + }, + core::type::Manager::StructMemberDesc{ + mod.symbols.New("point_size"), + ty.f32(), + BuiltinAttrs(core::BuiltinValue::kPointSize), + }, + }); + auto* builtins = b.Var("builtins", ty.ptr(core::AddressSpace::kOut, builtin_str)); + mod.root_block->Append(builtins); + + auto* ep = b.Function("foo", ty.void_(), core::ir::Function::PipelineStage::kVertex); + b.Append(ep->Block(), [&] { // + auto* ptr = ty.ptr(core::AddressSpace::kOut, ty.vec4<f32>()); + b.Store(b.Access(ptr, builtins, 0_u), b.Splat<vec4<f32>>(1_f)); + b.Store(b.Access(ty.ptr(core::AddressSpace::kOut, ty.f32()), builtins, 1_u), 1_f); + b.Return(ep); + }); + + auto* src = R"( +Builtins = struct @align(16) { + position:vec4<f32> @offset(0), @builtin(position) + point_size:f32 @offset(16), @builtin(__point_size) +} + +$B1: { # root + %builtins:ptr<__out, Builtins, read_write> = var +} + +%foo = @vertex func():void { + $B2: { + %3:ptr<__out, vec4<f32>, read_write> = access %builtins, 0u + store %3, vec4<f32>(1.0f) + %4:ptr<__out, f32, read_write> = access %builtins, 1u + store %4, 1.0f + ret + } +} +)"; + EXPECT_EQ(src, str()); + + auto* expect = R"( +Builtins = struct @align(16) { + position:vec4<f32> @offset(0) + point_size:f32 @offset(16) +} + +$B1: { # root + %builtins:ptr<private, Builtins, read_write> = var +} + +%foo_inner = func():void { + $B2: { + %3:ptr<private, vec4<f32>, read_write> = access %builtins, 0u + store %3, vec4<f32>(1.0f) + %4:ptr<private, f32, read_write> = access %builtins, 1u + store %4, 1.0f + ret + } +} +%foo = @vertex func():vec4<f32> [@position] { + $B3: { + %6:void = call %foo_inner + %7:ptr<private, vec4<f32>, read_write> = access %builtins, 0u + %8:vec4<f32> = load %7 + ret %8 + } +} +)"; + + Run(ShaderIO); + + EXPECT_EQ(expect, str()); +} + +TEST_F(SpirvReader_ShaderIOTest, Outputs_Struct_UnusedOutputs) { + auto* builtin_str = ty.Struct(mod.symbols.New("Builtins"), + Vector{ + core::type::Manager::StructMemberDesc{ + mod.symbols.New("position"), + ty.vec4<f32>(), + BuiltinAttrs(core::BuiltinValue::kPosition), + }, + core::type::Manager::StructMemberDesc{ + mod.symbols.New("clip_distance"), + ty.array<f32, 3>(), + BuiltinAttrs(core::BuiltinValue::kClipDistances), + }, + core::type::Manager::StructMemberDesc{ + mod.symbols.New("point_size"), + ty.f32(), + BuiltinAttrs(core::BuiltinValue::kPointSize), + }, + core::type::Manager::StructMemberDesc{ + mod.symbols.New("cull_distance"), + ty.array<f32, 3>(), + BuiltinAttrs(core::BuiltinValue::kCullDistance), + }, + }); + auto* builtins = b.Var("builtins", ty.ptr(core::AddressSpace::kOut, builtin_str)); + mod.root_block->Append(builtins); + + auto* ep = b.Function("foo", ty.void_(), core::ir::Function::PipelineStage::kVertex); + b.Append(ep->Block(), [&] { // + auto* ptr = ty.ptr(core::AddressSpace::kOut, ty.vec4<f32>()); + b.Store(b.Access(ptr, builtins, 0_u), b.Splat<vec4<f32>>(1_f)); + b.Return(ep); + }); + + auto* src = R"( +Builtins = struct @align(16) { + position:vec4<f32> @offset(0), @builtin(position) + clip_distance:array<f32, 3> @offset(16), @builtin(clip_distances) + point_size:f32 @offset(28), @builtin(__point_size) + cull_distance:array<f32, 3> @offset(32), @builtin(__cull_distance) +} + +$B1: { # root + %builtins:ptr<__out, Builtins, read_write> = var +} + +%foo = @vertex func():void { + $B2: { + %3:ptr<__out, vec4<f32>, read_write> = access %builtins, 0u + store %3, vec4<f32>(1.0f) + ret + } +} +)"; + EXPECT_EQ(src, str()); + + auto* expect = R"( +Builtins = struct @align(16) { + position:vec4<f32> @offset(0) + clip_distance:array<f32, 3> @offset(16) + point_size:f32 @offset(28) + cull_distance:array<f32, 3> @offset(32) +} + +$B1: { # root + %builtins:ptr<private, Builtins, read_write> = var +} + +%foo_inner = func():void { + $B2: { + %3:ptr<private, vec4<f32>, read_write> = access %builtins, 0u + store %3, vec4<f32>(1.0f) + ret + } +} +%foo = @vertex func():vec4<f32> [@position] { + $B3: { + %5:void = call %foo_inner + %6:ptr<private, vec4<f32>, read_write> = access %builtins, 0u + %7:vec4<f32> = load %6 + ret %7 + } +} +)"; + + Run(ShaderIO); + + EXPECT_EQ(expect, str()); +} + +TEST_F(SpirvReader_ShaderIOTest, Outputs_Struct_MultipledUsed) { + auto* builtin_str = ty.Struct(mod.symbols.New("Builtins"), + Vector{ + core::type::Manager::StructMemberDesc{ + mod.symbols.New("position"), + ty.vec4<f32>(), + BuiltinAttrs(core::BuiltinValue::kPosition), + }, + core::type::Manager::StructMemberDesc{ + mod.symbols.New("clip_distance"), + ty.array<f32, 3>(), + BuiltinAttrs(core::BuiltinValue::kClipDistances), + }, + }); + auto* builtins = b.Var("builtins", ty.ptr(core::AddressSpace::kOut, builtin_str)); + mod.root_block->Append(builtins); + + auto* ep = b.Function("foo", ty.void_(), core::ir::Function::PipelineStage::kVertex); + b.Append(ep->Block(), [&] { // + auto* ptr1 = ty.ptr(core::AddressSpace::kOut, ty.vec4<f32>()); + b.Store(b.Access(ptr1, builtins, 0_u), b.Splat<vec4<f32>>(1_f)); + + auto* ptr2 = ty.ptr(core::AddressSpace::kOut, ty.array<f32, 3>()); + b.Store(b.Access(ptr2, builtins, 1_u), b.Splat<array<f32, 3>>(1_f)); + b.Return(ep); + }); + + auto* src = R"( +Builtins = struct @align(16) { + position:vec4<f32> @offset(0), @builtin(position) + clip_distance:array<f32, 3> @offset(16), @builtin(clip_distances) +} + +$B1: { # root + %builtins:ptr<__out, Builtins, read_write> = var +} + +%foo = @vertex func():void { + $B2: { + %3:ptr<__out, vec4<f32>, read_write> = access %builtins, 0u + store %3, vec4<f32>(1.0f) + %4:ptr<__out, array<f32, 3>, read_write> = access %builtins, 1u + store %4, array<f32, 3>(1.0f) + ret + } +} +)"; + EXPECT_EQ(src, str()); + + auto* expect = R"( +Builtins = struct @align(16) { + position:vec4<f32> @offset(0) + clip_distance:array<f32, 3> @offset(16) +} + +tint_symbol = struct @align(16) { + position:vec4<f32> @offset(0), @builtin(position) + clip_distance:array<f32, 3> @offset(16), @builtin(clip_distances) +} + +$B1: { # root + %builtins:ptr<private, Builtins, read_write> = var +} + +%foo_inner = func():void { + $B2: { + %3:ptr<private, vec4<f32>, read_write> = access %builtins, 0u + store %3, vec4<f32>(1.0f) + %4:ptr<private, array<f32, 3>, read_write> = access %builtins, 1u + store %4, array<f32, 3>(1.0f) + ret + } +} +%foo = @vertex func():tint_symbol { + $B3: { + %6:void = call %foo_inner + %7:ptr<private, vec4<f32>, read_write> = access %builtins, 0u + %8:vec4<f32> = load %7 + %9:ptr<private, array<f32, 3>, read_write> = access %builtins, 1u + %10:array<f32, 3> = load %9 + %11:tint_symbol = construct %8, %10 + ret %11 + } +} +)"; + + Run(ShaderIO); + + EXPECT_EQ(expect, str()); +} } // namespace } // namespace tint::spirv::reader::lower
diff --git a/src/tint/lang/spirv/reader/parser/parser.cc b/src/tint/lang/spirv/reader/parser/parser.cc index 271973b..c43fe01 100644 --- a/src/tint/lang/spirv/reader/parser/parser.cc +++ b/src/tint/lang/spirv/reader/parser/parser.cc
@@ -155,6 +155,8 @@ return core::BuiltinValue::kWorkgroupId; case spv::BuiltIn::ClipDistance: return core::BuiltinValue::kClipDistances; + case spv::BuiltIn::CullDistance: + return core::BuiltinValue::kCullDistance; default: TINT_UNIMPLEMENTED() << "unhandled SPIR-V BuiltIn: " << static_cast<uint32_t>(b); }