writer/spirv: Fix constant initialization The code to auto-generate initializers for overridable pipeline constants was in the non-const code path, so move it to the right place and fix the tests that were wrongly testing non-const variables. Bug: tint:254 Change-Id: Ifc96681492768ecf844f67e114377cc643ef7609 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/50040 Auto-Submit: James Price <jrprice@google.com> Commit-Queue: Ben Clayton <bclayton@google.com> Reviewed-by: Ben Clayton <bclayton@google.com>
diff --git a/src/writer/spirv/builder.cc b/src/writer/spirv/builder.cc index 27300ff..8a74ad5 100644 --- a/src/writer/spirv/builder.cc +++ b/src/writer/spirv/builder.cc
@@ -688,8 +688,33 @@ if (var->is_const()) { if (!var->has_constructor()) { - error_ = "missing constructor for constant"; - return false; + // Constants must have an initializer unless they have an override + // decoration. + if (!ast::HasDecoration<ast::OverrideDecoration>(var->decorations())) { + error_ = "missing constructor for constant"; + return false; + } + + // SPIR-V requires specialization constants to have initializers. + if (sem->Type()->Is<sem::F32>()) { + ast::FloatLiteral l(ProgramID(), Source{}, 0.0f); + init_id = GenerateLiteralIfNeeded(var, &l); + } else if (sem->Type()->Is<sem::U32>()) { + ast::UintLiteral l(ProgramID(), Source{}, 0); + init_id = GenerateLiteralIfNeeded(var, &l); + } else if (sem->Type()->Is<sem::I32>()) { + ast::SintLiteral l(ProgramID(), Source{}, 0); + init_id = GenerateLiteralIfNeeded(var, &l); + } else if (sem->Type()->Is<sem::Bool>()) { + ast::BoolLiteral l(ProgramID(), Source{}, false); + init_id = GenerateLiteralIfNeeded(var, &l); + } else { + error_ = "invalid type for pipeline constant ID, must be scalar"; + return false; + } + if (init_id == 0) { + return 0; + } } push_debug(spv::Op::OpName, {Operand::Int(init_id), @@ -743,37 +768,10 @@ } } } else if (!type_no_ac->Is<sem::Sampler>()) { - // Certain cases require us to generate a constructor value. - // - // 1- Pipeline constant IDs must be attached to the OpConstant, if we have a - // variable with an override attribute that doesn't have a constructor we - // make one - // 2- If we don't have a constructor and we're an Output or Private variable - // then WGSL requires an initializer. - if (ast::HasDecoration<ast::OverrideDecoration>(var->decorations())) { - if (type_no_ac->Is<sem::F32>()) { - ast::FloatLiteral l(ProgramID(), Source{}, 0.0f); - init_id = GenerateLiteralIfNeeded(var, &l); - } else if (type_no_ac->Is<sem::U32>()) { - ast::UintLiteral l(ProgramID(), Source{}, 0); - init_id = GenerateLiteralIfNeeded(var, &l); - } else if (type_no_ac->Is<sem::I32>()) { - ast::SintLiteral l(ProgramID(), Source{}, 0); - init_id = GenerateLiteralIfNeeded(var, &l); - } else if (type_no_ac->Is<sem::Bool>()) { - ast::BoolLiteral l(ProgramID(), Source{}, false); - init_id = GenerateLiteralIfNeeded(var, &l); - } else { - error_ = "invalid type for pipeline constant ID, must be scalar"; - return false; - } - if (init_id == 0) { - return 0; - } - ops.push_back(Operand::Int(init_id)); - } else if (sem->StorageClass() == ast::StorageClass::kPrivate || - sem->StorageClass() == ast::StorageClass::kNone || - sem->StorageClass() == ast::StorageClass::kOutput) { + // If we don't have a constructor and we're an Output or Private variable, + // then WGSL requires that we zero-initialize. + if (sem->StorageClass() == ast::StorageClass::kPrivate || + sem->StorageClass() == ast::StorageClass::kOutput) { init_id = GenerateConstantNullIfNeeded(type_no_ac); if (init_id == 0) { return 0;
diff --git a/src/writer/spirv/builder_global_variable_test.cc b/src/writer/spirv/builder_global_variable_test.cc index 51a5b9d..c264fc5 100644 --- a/src/writer/spirv/builder_global_variable_test.cc +++ b/src/writer/spirv/builder_global_variable_test.cc
@@ -203,123 +203,111 @@ )"); } -TEST_F(BuilderTest, GlobalVar_ConstantId_Bool) { - auto* v = Global("var", ty.bool_(), ast::StorageClass::kInput, Expr(true), - ast::DecorationList{ - create<ast::OverrideDecoration>(1200), - }); +TEST_F(BuilderTest, GlobalVar_Override_Bool) { + auto* v = GlobalConst("var", ty.bool_(), Expr(true), + ast::DecorationList{ + create<ast::OverrideDecoration>(1200), + }); spirv::Builder& b = Build(); EXPECT_TRUE(b.GenerateGlobalVariable(v)) << b.error(); - EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %3 "var" + EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %2 "var" )"); EXPECT_EQ(DumpInstructions(b.annots()), R"(OpDecorate %2 SpecId 1200 )"); EXPECT_EQ(DumpInstructions(b.types()), R"(%1 = OpTypeBool %2 = OpSpecConstantTrue %1 -%4 = OpTypePointer Input %1 -%3 = OpVariable %4 Input %2 )"); } -TEST_F(BuilderTest, GlobalVar_ConstantId_Bool_NoConstructor) { - auto* v = Global("var", ty.bool_(), ast::StorageClass::kInput, nullptr, - ast::DecorationList{ - create<ast::OverrideDecoration>(1200), - }); +TEST_F(BuilderTest, GlobalVar_Override_Bool_NoConstructor) { + auto* v = GlobalConst("var", ty.bool_(), nullptr, + ast::DecorationList{ + create<ast::OverrideDecoration>(1200), + }); spirv::Builder& b = Build(); EXPECT_TRUE(b.GenerateGlobalVariable(v)) << b.error(); - EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %1 "var" + EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %2 "var" )"); - EXPECT_EQ(DumpInstructions(b.annots()), R"(OpDecorate %4 SpecId 1200 + EXPECT_EQ(DumpInstructions(b.annots()), R"(OpDecorate %2 SpecId 1200 )"); - EXPECT_EQ(DumpInstructions(b.types()), R"(%3 = OpTypeBool -%2 = OpTypePointer Input %3 -%4 = OpSpecConstantFalse %3 -%1 = OpVariable %2 Input %4 + EXPECT_EQ(DumpInstructions(b.types()), R"(%1 = OpTypeBool +%2 = OpSpecConstantFalse %1 )"); } -TEST_F(BuilderTest, GlobalVar_ConstantId_Scalar) { - auto* v = Global("var", ty.f32(), ast::StorageClass::kInput, Expr(2.f), - ast::DecorationList{ - create<ast::OverrideDecoration>(0), - }); +TEST_F(BuilderTest, GlobalVar_Override_Scalar) { + auto* v = GlobalConst("var", ty.f32(), Expr(2.f), + ast::DecorationList{ + create<ast::OverrideDecoration>(0), + }); spirv::Builder& b = Build(); EXPECT_TRUE(b.GenerateGlobalVariable(v)) << b.error(); - EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %3 "var" + EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %2 "var" )"); EXPECT_EQ(DumpInstructions(b.annots()), R"(OpDecorate %2 SpecId 0 )"); EXPECT_EQ(DumpInstructions(b.types()), R"(%1 = OpTypeFloat 32 %2 = OpSpecConstant %1 2 -%4 = OpTypePointer Input %1 -%3 = OpVariable %4 Input %2 )"); } -TEST_F(BuilderTest, GlobalVar_ConstantId_Scalar_F32_NoConstructor) { - auto* v = Global("var", ty.f32(), ast::StorageClass::kInput, nullptr, - ast::DecorationList{ - create<ast::OverrideDecoration>(0), - }); +TEST_F(BuilderTest, GlobalVar_Override_Scalar_F32_NoConstructor) { + auto* v = GlobalConst("var", ty.f32(), nullptr, + ast::DecorationList{ + create<ast::OverrideDecoration>(0), + }); spirv::Builder& b = Build(); EXPECT_TRUE(b.GenerateGlobalVariable(v)) << b.error(); - EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %1 "var" + EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %2 "var" )"); - EXPECT_EQ(DumpInstructions(b.annots()), R"(OpDecorate %4 SpecId 0 + EXPECT_EQ(DumpInstructions(b.annots()), R"(OpDecorate %2 SpecId 0 )"); - EXPECT_EQ(DumpInstructions(b.types()), R"(%3 = OpTypeFloat 32 -%2 = OpTypePointer Input %3 -%4 = OpSpecConstant %3 0 -%1 = OpVariable %2 Input %4 + EXPECT_EQ(DumpInstructions(b.types()), R"(%1 = OpTypeFloat 32 +%2 = OpSpecConstant %1 0 )"); } -TEST_F(BuilderTest, GlobalVar_ConstantId_Scalar_I32_NoConstructor) { - auto* v = Global("var", ty.i32(), ast::StorageClass::kInput, nullptr, - ast::DecorationList{ - create<ast::OverrideDecoration>(0), - }); +TEST_F(BuilderTest, GlobalVar_Override_Scalar_I32_NoConstructor) { + auto* v = GlobalConst("var", ty.i32(), nullptr, + ast::DecorationList{ + create<ast::OverrideDecoration>(0), + }); spirv::Builder& b = Build(); EXPECT_TRUE(b.GenerateGlobalVariable(v)) << b.error(); - EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %1 "var" + EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %2 "var" )"); - EXPECT_EQ(DumpInstructions(b.annots()), R"(OpDecorate %4 SpecId 0 + EXPECT_EQ(DumpInstructions(b.annots()), R"(OpDecorate %2 SpecId 0 )"); - EXPECT_EQ(DumpInstructions(b.types()), R"(%3 = OpTypeInt 32 1 -%2 = OpTypePointer Input %3 -%4 = OpSpecConstant %3 0 -%1 = OpVariable %2 Input %4 + EXPECT_EQ(DumpInstructions(b.types()), R"(%1 = OpTypeInt 32 1 +%2 = OpSpecConstant %1 0 )"); } -TEST_F(BuilderTest, GlobalVar_ConstantId_Scalar_U32_NoConstructor) { - auto* v = Global("var", ty.u32(), ast::StorageClass::kInput, nullptr, - ast::DecorationList{ - create<ast::OverrideDecoration>(0), - }); +TEST_F(BuilderTest, GlobalVar_Override_Scalar_U32_NoConstructor) { + auto* v = GlobalConst("var", ty.u32(), nullptr, + ast::DecorationList{ + create<ast::OverrideDecoration>(0), + }); spirv::Builder& b = Build(); EXPECT_TRUE(b.GenerateGlobalVariable(v)) << b.error(); - EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %1 "var" + EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %2 "var" )"); - EXPECT_EQ(DumpInstructions(b.annots()), R"(OpDecorate %4 SpecId 0 + EXPECT_EQ(DumpInstructions(b.annots()), R"(OpDecorate %2 SpecId 0 )"); - EXPECT_EQ(DumpInstructions(b.types()), R"(%3 = OpTypeInt 32 0 -%2 = OpTypePointer Input %3 -%4 = OpSpecConstant %3 0 -%1 = OpVariable %2 Input %4 + EXPECT_EQ(DumpInstructions(b.types()), R"(%1 = OpTypeInt 32 0 +%2 = OpSpecConstant %1 0 )"); }