diff --git a/src/reader/spirv/namer.h b/src/reader/spirv/namer.h index e4ee8c4606..1d231f4bbe 100644 --- a/src/reader/spirv/namer.h +++ b/src/reader/spirv/namer.h @@ -108,8 +108,8 @@ class Namer { /// @returns true if the ID did not have a previously registered name. bool Register(uint32_t id, const std::string& name); - /// Registers a name, but not associated to any ID. Fails if the name - /// was already registered. + /// Registers a name, but not associated to any ID. Fails and emits + /// a diagnostic if the name was already registered. /// @param name the name to register /// @returns true if the name was not already reegistered. bool RegisterWithoutId(const std::string& name); diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc index cfcf98618c..729beb4db9 100644 --- a/src/reader/spirv/parser_impl.cc +++ b/src/reader/spirv/parser_impl.cc @@ -621,7 +621,36 @@ bool ParserImpl::RegisterUserAndStructMemberNames() { module_->entry_points()) { const uint32_t function_id = entry_point.GetSingleWordInOperand(1); const std::string name = entry_point.GetInOperand(2).AsString(); + + // This translator requires the entry point to be a valid WGSL identifier. + // Allowing otherwise leads to difficulties in that the programmer needs + // to get a mapping from their original entry point name to the WGSL name, + // and we don't have a good mechanism for that. + if (!IsValidIdentifier(name)) { + return Fail() << "entry point name is not a valid WGSL identifier: " + << name; + } + + // SPIR-V allows a single function to be the implementation for more + // than one entry point. In the common case, it's one-to-one, and we should + // try to name the function after the entry point. Otherwise, give the + // function a name automatically derived from the entry point name. namer_.SuggestSanitizedName(function_id, name); + + // There is another many-to-one relationship to take care of: In SPIR-V + // the same name can be used for multiple entry points, provided they are + // for different shader stages. Take action now to ensure we can use the + // entry point name later on, and not have it taken for another identifier + // by an accidental collision with a derived name made for a different ID. + if (!namer_.IsRegistered(name)) { + // The entry point name is "unoccupied" becase an earlier entry point + // grabbed the slot for the function that implements both entry points. + // Register this new entry point's name, to avoid accidental collisions + // with a future generated ID. + if (!namer_.RegisterWithoutId(name)) { + return false; + } + } } // Register names from OpName and OpMemberName @@ -683,11 +712,6 @@ bool ParserImpl::RegisterEntryPoints() { const std::string ep_name = entry_point.GetOperand(2).AsString(); EntryPointInfo info{ep_name, enum_converter_.ToPipelineStage(stage)}; - if (!IsValidIdentifier(ep_name)) { - return Fail() << "entry point name is not a valid WGSL identifier: " - << ep_name; - } - function_to_ep_info_[function_id].push_back(info); } // The enum conversion could have failed, so return the existing status value. diff --git a/src/reader/spirv/parser_impl_user_name_test.cc b/src/reader/spirv/parser_impl_user_name_test.cc index 4c0922993e..d854a9b878 100644 --- a/src/reader/spirv/parser_impl_user_name_test.cc +++ b/src/reader/spirv/parser_impl_user_name_test.cc @@ -100,6 +100,42 @@ TEST_F(SpvParserTest, UserName_MemberNamesMixUserAndSynthesized) { EXPECT_THAT(p->namer().GetMemberName(3, 2), Eq("field2")); } +TEST_F(SpvParserTest, EntryPointNamesAlwaysTakePrecedence) { + const std::string assembly = R"( + OpCapability Shader + OpMemoryModel Logical Simple + OpEntryPoint GLCompute %100 "main" + OpEntryPoint Fragment %100 "main_1" + + ; attempt to grab the "main_1" that would be the derived name + ; for the second entry point. + OpName %1 "main_1" + + %void = OpTypeVoid + %voidfn = OpTypeFunction %void + %uint = OpTypeInt 32 0 + %uint_0 = OpConstant %uint 0 + + %100 = OpFunction %void None %voidfn + %100_entry = OpLabel + %1 = OpCopyObject %uint %uint_0 + OpReturn + OpFunctionEnd +)"; + auto p = parser(test::Assemble(assembly)); + EXPECT_TRUE(p->BuildAndParseInternalModule()); + // The first entry point grabs the best name, "main" + EXPECT_THAT(p->namer().Name(100), Eq("main")); + // The OpName on %1 is overriden becuase the second entry point + // has grabbed "main_1" first. + EXPECT_THAT(p->namer().Name(1), Eq("main_1_1")); + + const auto ep_info = p->GetEntryPointInfo(100); + ASSERT_EQ(2u, ep_info.size()); + EXPECT_EQ(ep_info[0].name, "main"); + EXPECT_EQ(ep_info[1].name, "main_1"); +} + } // namespace } // namespace spirv } // namespace reader