spirv-reader: always reserve entry point names

We have to be careful when the same function is used for
two entry points.  The first entry point name will be registered
as the name for the function ID.  But subsequent entry points
should have their names reserved in the namer, even if they
aren't associated with an ID.

Bug: tint:508
Change-Id: I3b6e7770ce49aa1b73594e57bdda5800febd55ed
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48864
Commit-Queue: David Neto <dneto@google.com>
Auto-Submit: David Neto <dneto@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>

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
+  /// Registers a name, but not associated to any ID. Fails and emits
-  /// was already registered.
+  /// 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);

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.

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