From ea855a9144481e8d87867fda34baa46a1e4420ff Mon Sep 17 00:00:00 2001 From: James Price Date: Thu, 25 Mar 2021 20:16:48 +0000 Subject: [PATCH] [spirv-writer] Handle non-pointer struct member accessors These map to OpCompositeExtract instructions. Fixed: tint:662 Change-Id: Ibd865bdb16326de7932157cbdfe543394415b3ff Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/45940 Auto-Submit: James Price Kokoro: Kokoro Reviewed-by: Ben Clayton Reviewed-by: David Neto Commit-Queue: James Price --- src/writer/spirv/builder.cc | 43 ++++--- .../spirv/builder_accessor_expression_test.cc | 113 +++++++++++++----- 2 files changed, 112 insertions(+), 44 deletions(-) diff --git a/src/writer/spirv/builder.cc b/src/writer/spirv/builder.cc index bec06069a4..440699a1cb 100644 --- a/src/writer/spirv/builder.cc +++ b/src/writer/spirv/builder.cc @@ -861,30 +861,43 @@ bool Builder::GenerateMemberAccessor(ast::MemberAccessorExpression* expr, // If the data_type is a structure we're accessing a member, if it's a // vector we're accessing a swizzle. if (data_type->Is()) { - if (!info->source_type->Is()) { - error_ = - "Attempting to access a struct member on a non-pointer. Something is " - "wrong"; - return false; - } - auto* strct = data_type->As()->impl(); auto symbol = expr->member()->symbol(); - uint32_t i = 0; - for (; i < strct->members().size(); ++i) { - auto* member = strct->members()[i]; + uint32_t idx = 0; + for (; idx < strct->members().size(); ++idx) { + auto* member = strct->members()[idx]; if (member->symbol() == symbol) { break; } } - auto idx_id = GenerateConstantIfNeeded(ScalarConstant::U32(i)); - if (idx_id == 0) { - return 0; + if (info->source_type->Is()) { + auto idx_id = GenerateConstantIfNeeded(ScalarConstant::U32(idx)); + if (idx_id == 0) { + return 0; + } + info->access_chain_indices.push_back(idx_id); + info->source_type = expr_type; + } else { + auto result_type_id = GenerateTypeIfNeeded(expr_type); + if (result_type_id == 0) { + return false; + } + + auto extract = result_op(); + auto extract_id = extract.to_i(); + if (!push_function_inst( + spv::Op::OpCompositeExtract, + {Operand::Int(result_type_id), extract, + Operand::Int(info->source_id), Operand::Int(idx)})) { + return false; + } + + info->source_id = extract_id; + info->source_type = expr_type; } - info->access_chain_indices.push_back(idx_id); - info->source_type = expr_type; + return true; } diff --git a/src/writer/spirv/builder_accessor_expression_test.cc b/src/writer/spirv/builder_accessor_expression_test.cc index 78ec1ad316..accad83457 100644 --- a/src/writer/spirv/builder_accessor_expression_test.cc +++ b/src/writer/spirv/builder_accessor_expression_test.cc @@ -255,6 +255,7 @@ TEST_F(BuilderTest, MemberAccessor) { TEST_F(BuilderTest, MemberAccessor_Nested) { // inner_struct { // a : f32 + // b : f32 // } // my_struct { // inner : inner_struct @@ -270,7 +271,7 @@ TEST_F(BuilderTest, MemberAccessor_Nested) { auto* s_type = Structure("my_struct", {Member("inner", inner_struct)}); auto* var = Global("ident", s_type, ast::StorageClass::kFunction); - auto* expr = MemberAccessor(MemberAccessor("ident", "inner"), "a"); + auto* expr = MemberAccessor(MemberAccessor("ident", "inner"), "b"); WrapInFunction(expr); spirv::Builder& b = Build(); @@ -278,7 +279,7 @@ TEST_F(BuilderTest, MemberAccessor_Nested) { b.push_function(Function{}); ASSERT_TRUE(b.GenerateFunctionVariable(var)) << b.error(); - EXPECT_EQ(b.GenerateAccessorExpression(expr), 10u); + EXPECT_EQ(b.GenerateAccessorExpression(expr), 11u); EXPECT_EQ(DumpInstructions(b.types()), R"(%5 = OpTypeFloat 32 %4 = OpTypeStruct %5 %5 @@ -287,13 +288,92 @@ TEST_F(BuilderTest, MemberAccessor_Nested) { %6 = OpConstantNull %3 %7 = OpTypeInt 32 0 %8 = OpConstant %7 0 -%9 = OpTypePointer Function %5 +%9 = OpConstant %7 1 +%10 = OpTypePointer Function %5 )"); EXPECT_EQ(DumpInstructions(b.functions()[0].variables()), R"(%1 = OpVariable %2 Function %6 )"); EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()), - R"(%10 = OpAccessChain %9 %1 %8 %8 + R"(%11 = OpAccessChain %10 %1 %8 %9 +)"); +} + +TEST_F(BuilderTest, MemberAccessor_NonPointer) { + // my_struct { + // a : f32 + // b : f32 + // } + // const ident : my_struct = my_struct(); + // ident.b + + auto* s = Structure("my_struct", { + Member("a", ty.f32()), + Member("b", ty.f32()), + }); + + auto* var = GlobalConst("ident", s, Construct(s, 0.f, 0.f)); + + auto* expr = MemberAccessor("ident", "b"); + WrapInFunction(expr); + + spirv::Builder& b = Build(); + + b.push_function(Function{}); + ASSERT_TRUE(b.GenerateFunctionVariable(var)) << b.error(); + + EXPECT_EQ(b.GenerateAccessorExpression(expr), 5u); + + EXPECT_EQ(DumpInstructions(b.types()), R"(%2 = OpTypeFloat 32 +%1 = OpTypeStruct %2 %2 +%3 = OpConstant %2 0 +%4 = OpConstantComposite %1 %3 %3 +)"); + EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()), + R"(%5 = OpCompositeExtract %2 %4 1 +)"); +} + +TEST_F(BuilderTest, MemberAccessor_Nested_NonPointer) { + // inner_struct { + // a : f32 + // b : f32 + // } + // my_struct { + // inner : inner_struct + // } + // + // const ident : my_struct = my_struct(); + // ident.inner.a + auto* inner_struct = Structure("Inner", { + Member("a", ty.f32()), + Member("b", ty.f32()), + }); + + auto* s_type = Structure("my_struct", {Member("inner", inner_struct)}); + + auto* var = GlobalConst("ident", s_type, + Construct(s_type, Construct(inner_struct, 0.f, 0.f))); + auto* expr = MemberAccessor(MemberAccessor("ident", "inner"), "b"); + WrapInFunction(expr); + + spirv::Builder& b = Build(); + + b.push_function(Function{}); + ASSERT_TRUE(b.GenerateFunctionVariable(var)) << b.error(); + + EXPECT_EQ(b.GenerateAccessorExpression(expr), 8u); + + EXPECT_EQ(DumpInstructions(b.types()), R"(%3 = OpTypeFloat 32 +%2 = OpTypeStruct %3 %3 +%1 = OpTypeStruct %2 +%4 = OpConstant %3 0 +%5 = OpConstantComposite %2 %4 %4 +%6 = OpConstantComposite %1 %5 +)"); + EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()), + R"(%7 = OpCompositeExtract %2 %6 0 +%8 = OpCompositeExtract %3 %7 1 )"); } @@ -753,31 +833,6 @@ TEST_F(BuilderTest, DISABLED_Accessor_Array_NonPointer) { // out of the ScalarConstructor as extract requires integer indices. } -TEST_F(BuilderTest, DISABLED_Accessor_Struct_NonPointer) { - // type A = struct { - // a : f32; - // b : f32; - // }; - // const b : A; - // b.b - // - // This needs to do an OpCompositeExtract on the struct. -} - -TEST_F(BuilderTest, DISABLED_Accessor_NonPointer_Multi) { - // type A = struct { - // a : f32; - // b : vec3; - // }; - // type B = struct { - // c : A; - // } - // const b : array; - // b[2].c.b.yx.x - // - // This needs to do an OpCompositeExtract similar to the AccessChain case -} - } // namespace } // namespace spirv } // namespace writer