Skip to content

Commit

Permalink
Avoid generating duplicate names when merging types
Browse files Browse the repository at this point in the history
The merging types we do not remove other information related to the
types.  We simply leave it duplicated, and hope it is removed later.
This is what happens with decorations.  They are removed in the next
phase of remove duplicates.  However, for OpNames that is not the case.
We end up with two different names for the same id, which does not make
sense.

The solution is to remove the names and decorations for the type being
removed instead of rewriting them to refer to the other type.

Note that it is possible that if the first type does not have a name,
then the types will end up with no name.  That is fine because the names
should not have any semantic significance anyway.

The was identified in issue #1372, but this does not fix that issue.
  • Loading branch information
s-perron committed Mar 5, 2018
1 parent 6cd6e5e commit 9ba50e3
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 0 deletions.
1 change: 1 addition & 0 deletions source/opt/remove_duplicates_pass.cpp
Expand Up @@ -130,6 +130,7 @@ bool RemoveDuplicatesPass::RemoveDuplicateTypes(
visited_types.emplace_back(i);
} else {
// The same type has already been seen before, remove this one.
ir_context->KillNamesAndDecorates(i->result_id());
ir_context->ReplaceAllUsesWith(i->result_id(), id_to_keep);
modified = true;
to_delete.emplace_back(i);
Expand Down
23 changes: 23 additions & 0 deletions test/opt/pass_remove_duplicates_test.cpp
Expand Up @@ -219,6 +219,29 @@ OpDecorate %1 GLSLPacked
EXPECT_THAT(GetErrorMessage(), "");
}

TEST_F(RemoveDuplicatesTest, SameTypeAndDifferentName) {
const std::string spirv = R"(
OpCapability Shader
OpCapability Linkage
OpMemoryModel Logical GLSL450
OpName %1 "Type1"
OpName %2 "Type2"
%3 = OpTypeInt 32 0
%1 = OpTypeStruct %3 %3
%2 = OpTypeStruct %3 %3
)";
const std::string after = R"(OpCapability Shader
OpCapability Linkage
OpMemoryModel Logical GLSL450
OpName %1 "Type1"
%3 = OpTypeInt 32 0
%1 = OpTypeStruct %3 %3
)";

EXPECT_THAT(RunPass(spirv), after);
EXPECT_THAT(GetErrorMessage(), "");
}

// Check that #1033 has been fixed.
TEST_F(RemoveDuplicatesTest, DoNotRemoveDifferentOpDecorationGroup) {
const std::string spirv = R"(
Expand Down

0 comments on commit 9ba50e3

Please sign in to comment.