From a696b027b3851596d03e33fb20418a52c2566480 Mon Sep 17 00:00:00 2001 From: James Price Date: Thu, 25 Mar 2021 20:49:37 +0000 Subject: [PATCH] [spirv-writer] Use const for function parameters The backend was wrongly generating OpLoad instructions for function parameter accesses since it thought they were pointers. Run SPIR-V validation for the entry point IO tests. Bug: tint:509 Change-Id: Ic816a22973fb87c7ede26bad8fb595764a333250 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/45941 Kokoro: Kokoro Reviewed-by: Ben Clayton Reviewed-by: David Neto Commit-Queue: James Price --- src/transform/spirv.cc | 8 +- src/transform/spirv_test.cc | 24 +-- src/writer/spirv/builder_entry_point_test.cc | 148 +++++++++---------- 3 files changed, 81 insertions(+), 99 deletions(-) diff --git a/src/transform/spirv.cc b/src/transform/spirv.cc index 2d0580a1e8..0f872d3f1b 100644 --- a/src/transform/spirv.cc +++ b/src/transform/spirv.cc @@ -156,8 +156,7 @@ void Spirv::HandleEntryPointIOTypes(CloneContext& ctx) const { // Create a function that writes a return value to all output variables. auto* store_value = - ctx.dst->Var(store_value_symbol, ctx.Clone(func->return_type()), - ast::StorageClass::kFunction, nullptr); + ctx.dst->Const(store_value_symbol, ctx.Clone(func->return_type())); auto return_func_symbol = ctx.dst->Symbols().New(); auto* return_func = ctx.dst->create( return_func_symbol, ast::VariableList{store_value}, @@ -268,11 +267,8 @@ Symbol Spirv::HoistToInputVariables( } // Create a function-scope variable for the struct. - // TODO(jrprice): Use Const when crbug.com/tint/662 is fixed auto* initializer = ctx.dst->Construct(ctx.Clone(ty), init_values); - auto* func_var = - ctx.dst->Var(func_var_symbol, ctx.Clone(ty), ast::StorageClass::kFunction, - initializer, ast::DecorationList{}); + auto* func_var = ctx.dst->Const(func_var_symbol, ctx.Clone(ty), initializer); ctx.InsertBefore(*func->body()->begin(), ctx.dst->WrapInStatement(func_var)); return func_var_symbol; } diff --git a/src/transform/spirv_test.cc b/src/transform/spirv_test.cc index d8d0d0996d..9df9ed29f1 100644 --- a/src/transform/spirv_test.cc +++ b/src/transform/spirv_test.cc @@ -214,7 +214,7 @@ struct FragmentInput { [[stage(fragment)]] fn frag_main() -> void { - var tint_symbol_6 : FragmentInput = FragmentInput(tint_symbol_4, tint_symbol_5); + const tint_symbol_6 : FragmentInput = FragmentInput(tint_symbol_4, tint_symbol_5); var col : f32 = (tint_symbol_6.coord.x * tint_symbol_6.value); } )"; @@ -282,10 +282,10 @@ struct FragmentInput { [[stage(fragment)]] fn frag_main() -> void { - var tint_symbol_13 : Builtins = Builtins(tint_symbol_12); - var tint_symbol_16 : Locations = Locations(tint_symbol_14, tint_symbol_15); - var tint_symbol_17 : Other = Other(tint_symbol_16); - var tint_symbol_19 : FragmentInput = FragmentInput(tint_symbol_13, tint_symbol_17, tint_symbol_18); + const tint_symbol_13 : Builtins = Builtins(tint_symbol_12); + const tint_symbol_16 : Locations = Locations(tint_symbol_14, tint_symbol_15); + const tint_symbol_17 : Other = Other(tint_symbol_16); + const tint_symbol_19 : FragmentInput = FragmentInput(tint_symbol_13, tint_symbol_17, tint_symbol_18); var col : f32 = (tint_symbol_19.b.coord.x * tint_symbol_19.value); var l : f32 = (tint_symbol_19.o.l.l2 + tint_symbol_19.o.l.l3); } @@ -499,7 +499,7 @@ fn tint_symbol_7(tint_symbol_5 : Interface) -> void { [[stage(vertex)]] fn vert_main() -> void { - var tint_symbol_4 : Interface = Interface(tint_symbol_3); + const tint_symbol_4 : Interface = Interface(tint_symbol_3); tint_symbol_7(tint_symbol_4); return; } @@ -548,7 +548,7 @@ fn vert_main() -> void { [[stage(fragment)]] fn frag_main() -> void { - var tint_symbol_8 : Interface = Interface(tint_symbol_7); + const tint_symbol_8 : Interface = Interface(tint_symbol_7); var x : f32 = tint_symbol_8.value; } )"; @@ -621,8 +621,8 @@ fn vert_main() -> void { [[stage(fragment)]] fn frag_main() -> void { - var tint_symbol_15 : Interface = Interface(tint_symbol_14); - var tint_symbol_16 : FragmentInput = FragmentInput(tint_symbol_13, tint_symbol_15); + const tint_symbol_15 : Interface = Interface(tint_symbol_14); + const tint_symbol_16 : FragmentInput = FragmentInput(tint_symbol_13, tint_symbol_15); var x : f32 = tint_symbol_16.interface.value; } )"; @@ -690,8 +690,8 @@ fn tint_symbol_17(tint_symbol_13 : MyVertexOutput) -> void { [[stage(vertex)]] fn vert_main() -> void { - var tint_symbol_9 : MyLocation = MyLocation(tint_symbol_8); - var tint_symbol_11 : MyVertexInput = MyVertexInput(tint_symbol_9, tint_symbol_10); + const tint_symbol_9 : MyLocation = MyLocation(tint_symbol_8); + const tint_symbol_11 : MyVertexInput = MyVertexInput(tint_symbol_9, tint_symbol_10); tint_symbol_17(tint_symbol_11); return; } @@ -746,7 +746,7 @@ fn tint_symbol_10(tint_symbol_8 : FragmentOutput) -> void { [[stage(fragment)]] fn frag_main() -> void { - var tint_symbol_7 : FragmentInput = FragmentInput(tint_symbol_5, tint_symbol_6); + const tint_symbol_7 : FragmentInput = FragmentInput(tint_symbol_5, tint_symbol_6); tint_symbol_10(FragmentOutput((tint_symbol_7.coord.x * tint_symbol_7.value))); return; } diff --git a/src/writer/spirv/builder_entry_point_test.cc b/src/writer/spirv/builder_entry_point_test.cc index 3650c3d8ec..f722d810ce 100644 --- a/src/writer/spirv/builder_entry_point_test.cc +++ b/src/writer/spirv/builder_entry_point_test.cc @@ -95,6 +95,8 @@ OpStore %17 %16 OpReturn OpFunctionEnd )"); + + Validate(b); } TEST_F(BuilderTest, EntryPoint_ReturnValue) { @@ -129,13 +131,13 @@ TEST_F(BuilderTest, EntryPoint_ReturnValue) { // Output storage class, and the return statements are replaced with stores. EXPECT_EQ(DumpBuilder(b), R"(OpCapability Shader OpMemoryModel Logical GLSL450 -OpEntryPoint Fragment %15 "frag_main" %1 %4 -OpExecutionMode %15 OriginUpperLeft +OpEntryPoint Fragment %14 "frag_main" %1 %4 +OpExecutionMode %14 OriginUpperLeft OpName %1 "tint_symbol_1" OpName %4 "tint_symbol_3" OpName %10 "tint_symbol_4" OpName %11 "tint_symbol_2" -OpName %15 "frag_main" +OpName %14 "frag_main" OpDecorate %1 Location 0 OpDecorate %4 Location 0 %3 = OpTypeInt 32 0 @@ -147,32 +149,33 @@ OpDecorate %4 Location 0 %4 = OpVariable %5 Output %7 %9 = OpTypeVoid %8 = OpTypeFunction %9 %6 -%14 = OpTypeFunction %9 -%18 = OpConstant %3 10 -%20 = OpTypeBool -%24 = OpConstant %6 0.5 -%26 = OpConstant %6 1 +%13 = OpTypeFunction %9 +%17 = OpConstant %3 10 +%19 = OpTypeBool +%23 = OpConstant %6 0.5 +%25 = OpConstant %6 1 %10 = OpFunction %9 None %8 %11 = OpFunctionParameter %6 %12 = OpLabel -%13 = OpLoad %6 %11 -OpStore %4 %13 +OpStore %4 %11 OpReturn OpFunctionEnd -%15 = OpFunction %9 None %14 -%16 = OpLabel -%17 = OpLoad %3 %1 -%19 = OpUGreaterThan %20 %17 %18 -OpSelectionMerge %21 None -OpBranchConditional %19 %22 %21 -%22 = OpLabel -%23 = OpFunctionCall %9 %10 %24 -OpReturn +%14 = OpFunction %9 None %13 +%15 = OpLabel +%16 = OpLoad %3 %1 +%18 = OpUGreaterThan %19 %16 %17 +OpSelectionMerge %20 None +OpBranchConditional %18 %21 %20 %21 = OpLabel -%25 = OpFunctionCall %9 %10 %26 +%22 = OpFunctionCall %9 %10 %23 +OpReturn +%20 = OpLabel +%24 = OpFunctionCall %9 %10 %25 OpReturn OpFunctionEnd )"); + + Validate(b); } TEST_F(BuilderTest, EntryPoint_SharedSubStruct) { @@ -250,10 +253,10 @@ TEST_F(BuilderTest, EntryPoint_SharedSubStruct) { EXPECT_EQ(DumpBuilder(b), R"(OpCapability Shader OpMemoryModel Logical GLSL450 -OpEntryPoint Vertex %30 "vert_main" %1 %6 -OpEntryPoint Fragment %41 "frag_main" %11 %9 %12 -OpExecutionMode %41 OriginUpperLeft -OpExecutionMode %41 DepthReplacing +OpEntryPoint Vertex %24 "vert_main" %1 %6 +OpEntryPoint Fragment %34 "frag_main" %11 %9 %12 +OpExecutionMode %34 OriginUpperLeft +OpExecutionMode %34 DepthReplacing OpName %1 "tint_symbol_9" OpName %6 "tint_symbol_10" OpName %9 "tint_symbol_13" @@ -266,15 +269,13 @@ OpName %16 "Interface" OpMemberName %16 0 "value" OpName %17 "tint_symbol_11" OpName %18 "tint_symbol_8" -OpName %30 "vert_main" -OpName %37 "tint_symbol_19" -OpName %38 "tint_symbol_17" -OpName %41 "frag_main" -OpName %45 "tint_symbol_15" -OpName %48 "FragmentInput" -OpMemberName %48 0 "mul" -OpMemberName %48 1 "interface" -OpName %52 "tint_symbol_16" +OpName %24 "vert_main" +OpName %31 "tint_symbol_19" +OpName %32 "tint_symbol_17" +OpName %34 "frag_main" +OpName %38 "FragmentInput" +OpMemberName %38 0 "mul" +OpMemberName %38 1 "interface" OpDecorate %1 BuiltIn Position OpDecorate %6 Location 1 OpDecorate %9 Location 0 @@ -283,8 +284,8 @@ OpDecorate %12 BuiltIn FragDepth OpMemberDecorate %15 0 Offset 0 OpMemberDecorate %15 1 Offset 16 OpMemberDecorate %16 0 Offset 0 -OpMemberDecorate %48 0 Offset 0 -OpMemberDecorate %48 1 Offset 4 +OpMemberDecorate %38 0 Offset 0 +OpMemberDecorate %38 1 Offset 4 %4 = OpTypeFloat 32 %3 = OpTypeVector %4 4 %2 = OpTypePointer Output %3 @@ -301,64 +302,49 @@ OpMemberDecorate %48 1 Offset 4 %16 = OpTypeStruct %4 %15 = OpTypeStruct %3 %16 %13 = OpTypeFunction %14 %15 -%20 = OpTypeInt 32 0 -%21 = OpConstant %20 0 -%22 = OpTypePointer Function %3 -%25 = OpConstant %20 1 -%26 = OpTypePointer Function %4 -%29 = OpTypeFunction %14 -%33 = OpConstant %4 42 -%34 = OpConstantComposite %16 %33 -%35 = OpConstantComposite %15 %5 %34 -%36 = OpTypeFunction %14 %4 -%46 = OpTypePointer Function %16 -%47 = OpConstantNull %16 -%48 = OpTypeStruct %4 %16 -%53 = OpTypePointer Function %48 -%54 = OpConstantNull %48 +%23 = OpTypeFunction %14 +%27 = OpConstant %4 42 +%28 = OpConstantComposite %16 %27 +%29 = OpConstantComposite %15 %5 %28 +%30 = OpTypeFunction %14 %4 +%38 = OpTypeStruct %4 %16 %17 = OpFunction %14 None %13 %18 = OpFunctionParameter %15 %19 = OpLabel -%23 = OpAccessChain %22 %18 %21 -%24 = OpLoad %3 %23 -OpStore %1 %24 -%27 = OpAccessChain %26 %18 %25 %21 -%28 = OpLoad %4 %27 -OpStore %6 %28 +%20 = OpCompositeExtract %3 %18 0 +OpStore %1 %20 +%21 = OpCompositeExtract %16 %18 1 +%22 = OpCompositeExtract %4 %21 0 +OpStore %6 %22 OpReturn OpFunctionEnd -%30 = OpFunction %14 None %29 -%31 = OpLabel -%32 = OpFunctionCall %14 %17 %35 +%24 = OpFunction %14 None %23 +%25 = OpLabel +%26 = OpFunctionCall %14 %17 %29 OpReturn OpFunctionEnd -%37 = OpFunction %14 None %36 -%38 = OpFunctionParameter %4 -%39 = OpLabel -%40 = OpLoad %4 %38 -OpStore %12 %40 +%31 = OpFunction %14 None %30 +%32 = OpFunctionParameter %4 +%33 = OpLabel +OpStore %12 %32 OpReturn OpFunctionEnd -%41 = OpFunction %14 None %29 -%42 = OpLabel -%45 = OpVariable %46 Function %47 -%52 = OpVariable %53 Function %54 -%43 = OpLoad %4 %11 -%44 = OpCompositeConstruct %16 %43 -OpStore %45 %44 -%49 = OpLoad %4 %9 -%50 = OpLoad %16 %45 -%51 = OpCompositeConstruct %48 %49 %50 -OpStore %52 %51 -%56 = OpAccessChain %26 %52 %21 -%57 = OpLoad %4 %56 -%58 = OpAccessChain %26 %52 %25 %21 -%59 = OpLoad %4 %58 -%60 = OpFMul %4 %57 %59 -%55 = OpFunctionCall %14 %37 %60 +%34 = OpFunction %14 None %23 +%35 = OpLabel +%36 = OpLoad %4 %11 +%37 = OpCompositeConstruct %16 %36 +%39 = OpLoad %4 %9 +%40 = OpCompositeConstruct %38 %39 %37 +%42 = OpCompositeExtract %4 %40 0 +%43 = OpCompositeExtract %16 %40 1 +%44 = OpCompositeExtract %4 %43 0 +%45 = OpFMul %4 %42 %44 +%41 = OpFunctionCall %14 %31 %45 OpReturn OpFunctionEnd )"); + + Validate(b); } } // namespace