Handle decoration groups with no decorations. (#1921)
In DecorationManager::RemoveDecorationsFrom, we do not remove the id
from a decoration group if the group has no decorations. This causes
problems because KillNamesAndDecorates is suppose to remove all
references to the id, but in this case, there is still a reference.
This is fixed by adding a special case.
Also, there is the possibility of a double free because
RemoveDecorationsFrom will delete the instructions defining |id| when
|id| is a decoration group. Later, KillInst would later write to memory
that has been deleted when trying to turn it into a Nop. To fix this,
we will only remove the decorations that use |id| and not its definition
in RemoveDecorationsFrom.
diff --git a/source/opt/decoration_manager.cpp b/source/opt/decoration_manager.cpp
index 82aa495..185dcb7 100644
--- a/source/opt/decoration_manager.cpp
+++ b/source/opt/decoration_manager.cpp
@@ -29,7 +29,9 @@
void DecorationManager::RemoveDecorationsFrom(
uint32_t id, std::function<bool(const Instruction&)> pred) {
const auto ids_iter = id_to_decoration_insts_.find(id);
- if (ids_iter == id_to_decoration_insts_.end()) return;
+ if (ids_iter == id_to_decoration_insts_.end()) {
+ return;
+ }
TargetData& decorations_info = ids_iter->second;
auto context = module_->context();
@@ -59,8 +61,14 @@
if (!pred(*decoration)) group_decorations_to_keep.push_back(decoration);
}
- // If all decorations should be kept, move to the next group
- if (group_decorations_to_keep.size() == group_decorations.size()) continue;
+ // If all decorations should be kept, then we can keep |id| part of the
+ // group. However, if the group itself has no decorations, we should remove
+ // the id from the group. This is needed to make |KillNameAndDecorate| work
+ // correctly when a decoration group has no decorations.
+ if (group_decorations_to_keep.size() == group_decorations.size() &&
+ group_decorations.size() != 0) {
+ continue;
+ }
// Otherwise, remove |id| from the targets of |group_id|
const uint32_t stride = inst->opcode() == SpvOpGroupDecorate ? 1u : 2u;
@@ -136,9 +144,6 @@
decorations_info.indirect_decorations.empty() &&
decorations_info.decorate_insts.empty()) {
id_to_decoration_insts_.erase(ids_iter);
-
- // Remove the OpDecorationGroup defining this group.
- if (is_group) context->KillInst(context->get_def_use_mgr()->GetDef(id));
}
}
diff --git a/source/opt/decoration_manager.h b/source/opt/decoration_manager.h
index a517ba2..fb9cfb6 100644
--- a/source/opt/decoration_manager.h
+++ b/source/opt/decoration_manager.h
@@ -36,11 +36,22 @@
}
DecorationManager() = delete;
- // Removes all decorations from |id| (either directly or indirectly) for
- // which |pred| returns true.
- // If |id| is a group ID, OpGroupDecorate and OpGroupMemberDecorate will be
- // removed if they have no targets left, and OpDecorationGroup will be
- // removed if the group is not applied to anyone and contains no decorations.
+ // Changes all of the decorations (direct and through groups) where |pred| is
+ // true and that apply to |id| so that they no longer apply to |id|.
+ //
+ // If |id| is part of a group, it will be removed from the group if it
+ // does not use all of the group's decorations, or, if there are no
+ // decorations that apply to the group.
+ //
+ // If decoration groups become empty, the |OpGroupDecorate| and
+ // |OpGroupMemberDecorate| instructions will be killed.
+ //
+ // Decoration instructions that apply directly to |id| will be killed.
+ //
+ // If |id| is a decoration group and all of the group's decorations are
+ // removed, then the |OpGroupDecorate| and
+ // |OpGroupMemberDecorate| for the group will be killed, but not the defining
+ // |OpDecorationGroup| instruction.
void RemoveDecorationsFrom(uint32_t id,
std::function<bool(const Instruction&)> pred =
[](const Instruction&) { return true; });
diff --git a/test/opt/decoration_manager_test.cpp b/test/opt/decoration_manager_test.cpp
index cf82e8e..f85ff6a 100644
--- a/test/opt/decoration_manager_test.cpp
+++ b/test/opt/decoration_manager_test.cpp
@@ -422,6 +422,7 @@
OpCapability Linkage
OpMemoryModel Logical GLSL450
OpDecorate %1 Constant
+%2 = OpDecorationGroup
%4 = OpTypeInt 32 0
%1 = OpVariable %4 Uniform
%3 = OpVariable %4 Uniform
diff --git a/test/opt/ir_context_test.cpp b/test/opt/ir_context_test.cpp
index 41e708e..10a92a7 100644
--- a/test/opt/ir_context_test.cpp
+++ b/test/opt/ir_context_test.cpp
@@ -282,6 +282,94 @@
EXPECT_EQ(i, localContext.TakeNextUniqueId());
}
+TEST_F(IRContextTest, KillGroupDecorationWitNoDecorations) {
+ const std::string text = R"(
+ OpCapability Shader
+ %1 = OpExtInstImport "GLSL.std.450"
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint Fragment %2 "main"
+ OpExecutionMode %2 OriginUpperLeft
+ OpSource GLSL 430
+ %3 = OpDecorationGroup
+ OpGroupDecorate %3 %4 %5
+ %6 = OpTypeFloat 32
+ %7 = OpTypePointer Function %6
+ %8 = OpTypeStruct %6
+ %9 = OpTypeVoid
+ %10 = OpTypeFunction %9
+ %2 = OpFunction %9 None %10
+ %11 = OpLabel
+ %4 = OpVariable %7 Function
+ %5 = OpVariable %7 Function
+ OpReturn
+ OpFunctionEnd
+)";
+
+ std::unique_ptr<IRContext> context =
+ BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text);
+
+ // Build the decoration manager.
+ context->get_decoration_mgr();
+
+ // Delete the second variable.
+ context->KillDef(5);
+
+ // The two decoration instructions should still be there. The first one
+ // should be the same, but the second should have %5 removed.
+
+ // Check the OpDecorationGroup Instruction
+ auto inst = context->annotation_begin();
+ EXPECT_EQ(inst->opcode(), SpvOpDecorationGroup);
+ EXPECT_EQ(inst->result_id(), 3);
+
+ // Check that %5 is no longer part of the group.
+ ++inst;
+ EXPECT_EQ(inst->opcode(), SpvOpGroupDecorate);
+ EXPECT_EQ(inst->NumInOperands(), 2);
+ EXPECT_EQ(inst->GetSingleWordInOperand(0), 3);
+ EXPECT_EQ(inst->GetSingleWordInOperand(1), 4);
+
+ // Check that we are at the end.
+ ++inst;
+ EXPECT_EQ(inst, context->annotation_end());
+}
+
+TEST_F(IRContextTest, KillDecorationGroup) {
+ const std::string text = R"(
+ OpCapability Shader
+ %1 = OpExtInstImport "GLSL.std.450"
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint Fragment %2 "main"
+ OpExecutionMode %2 OriginUpperLeft
+ OpSource GLSL 430
+ %3 = OpDecorationGroup
+ OpGroupDecorate %3 %4 %5
+ %6 = OpTypeFloat 32
+ %7 = OpTypePointer Function %6
+ %8 = OpTypeStruct %6
+ %9 = OpTypeVoid
+ %10 = OpTypeFunction %9
+ %2 = OpFunction %9 None %10
+ %11 = OpLabel
+ %4 = OpVariable %7 Function
+ %5 = OpVariable %7 Function
+ OpReturn
+ OpFunctionEnd
+)";
+
+ std::unique_ptr<IRContext> context =
+ BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text);
+
+ // Build the decoration manager.
+ context->get_decoration_mgr();
+
+ // Delete the second variable.
+ context->KillDef(3);
+
+ // Check the OpDecorationGroup Instruction is still there.
+ EXPECT_TRUE(context->annotations().empty());
+}
+
} // namespace
} // namespace opt
} // namespace spvtools
