AuroraMiddleware/SPIRV-Tools
1
0
Fork 0
mirror of https://github.com/KhronosGroup/SPIRV-Tools synced 2024-11-22 19:50:05 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fixing bugs in type manager memory management

Browse Source 
* changed the way duplicate types are removed to stop copying
instructions
* Reworked RemoveDuplicatesPass::AreTypesSame to use type manager and
type equality
* Reworked TypeManager memory management to store a pool of unique
pointers of types
 * removed unique pointers from id map
 * fixed instances where free'd memory could be accessed
This commit is contained in:
Alan Baker 2017-12-18 17:02:19 -05:00
parent 7505d24225
commit 1ab8ad654a
8 changed files with 190 additions and 156 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
source/link/linker.cpp
View File

@ -119,8 +119,7 @@ static spv_result_t GetImportExportPairs(
// checked.
static spv_result_t CheckImportExportCompatibility(
const MessageConsumer& consumer, const LinkageTable& linkings_to_do,
const DefUseManager& def_use_manager,
const DecorationManager& decoration_manager);
ir::IRContext* context);
// Remove linkage specific instructions, such as prototypes of imported
// functions, declarations of imported variables, import (and export if
@ -247,9 +246,8 @@ spv_result_t Linker::Link(const uint32_t* const* binaries,
if (res != SPV_SUCCESS) return res;
// Phase 5: Ensure the import and export have the same types and decorations.
res = CheckImportExportCompatibility(consumer, linkings_to_do,
*linked_context.get_def_use_mgr(),
*linked_context.get_decoration_mgr());
res =
CheckImportExportCompatibility(consumer, linkings_to_do, &linked_context);
if (res != SPV_SUCCESS) return res;
// Phase 6: Remove duplicates
@ -599,16 +597,17 @@ static spv_result_t GetImportExportPairs(
static spv_result_t CheckImportExportCompatibility(
const MessageConsumer& consumer, const LinkageTable& linkings_to_do,
const DefUseManager& def_use_manager,
const DecorationManager& decoration_manager) {
ir::IRContext* context) {
spv_position_t position = {};
// Ensure th import and export types are the same.
const DefUseManager& def_use_manager = *context->get_def_use_mgr();
const DecorationManager& decoration_manager = *context->get_decoration_mgr();
for (const auto& linking_entry : linkings_to_do) {
if (!RemoveDuplicatesPass::AreTypesEqual(
*def_use_manager.GetDef(linking_entry.imported_symbol.type_id),
*def_use_manager.GetDef(linking_entry.exported_symbol.type_id),
def_use_manager, decoration_manager))
context))
return libspirv::DiagnosticStream(position, consumer,
SPV_ERROR_INVALID_BINARY)
<< "Type mismatch between imported variable/function %"

124
source/opt/remove_duplicates_pass.cpp
View File

@ -25,6 +25,7 @@
#include "decoration_manager.h"
#include "ir_context.h"
#include "opcode.h"
#include "reflect.h"
namespace spvtools {
namespace opt {
@ -104,37 +105,39 @@ bool RemoveDuplicatesPass::RemoveDuplicateTypes(
return modified;
}
std::vector<Instruction> visitedTypes;
for (auto* i = &*irContext->types_values_begin(); i;) {
std::vector<Instruction*> visitedTypes;
std::vector<Instruction*> toDelete;
for (auto* i = &*irContext->types_values_begin(); i; i = i->NextNode()) {
// We only care about types.
if (!spvOpcodeGeneratesType((i->opcode())) &&
i->opcode() != SpvOpTypeForwardPointer) {
i = i->NextNode();
continue;
}
// Is the current type equal to one of the types we have aready visited?
SpvId idToKeep = 0u;
for (auto j : visitedTypes) {
if (AreTypesEqual(*i, j, *irContext->get_def_use_mgr(),
*irContext->get_decoration_mgr())) {
idToKeep = j.result_id();
if (AreTypesEqual(*i, *j, irContext)) {
idToKeep = j->result_id();
break;
}
}
if (idToKeep == 0u) {
// This is a never seen before type, keep it around.
visitedTypes.emplace_back(*i);
i = i->NextNode();
visitedTypes.emplace_back(i);
} else {
// The same type has already been seen before, remove this one.
irContext->ReplaceAllUsesWith(i->result_id(), idToKeep);
modified = true;
i = irContext->KillInst(i);
toDelete.emplace_back(i);
}
}
for (auto i : toDelete) {
irContext->KillInst(i);
}
return modified;
}
@ -181,102 +184,17 @@ bool RemoveDuplicatesPass::RemoveDuplicateDecorations(
bool RemoveDuplicatesPass::AreTypesEqual(const Instruction& inst1,
const Instruction& inst2,
const DefUseManager& defUseManager,
const DecorationManager& decManager) {
ir::IRContext* context) {
if (inst1.opcode() != inst2.opcode()) return false;
if (!decManager.HaveTheSameDecorations(inst1.result_id(), inst2.result_id()))
return false;
if (!ir::IsTypeInst(inst1.opcode())) return false;
switch (inst1.opcode()) {
case SpvOpTypeVoid:
case SpvOpTypeBool:
case SpvOpTypeSampler:
case SpvOpTypeEvent:
case SpvOpTypeDeviceEvent:
case SpvOpTypeReserveId:
case SpvOpTypeQueue:
case SpvOpTypePipeStorage:
case SpvOpTypeNamedBarrier:
return true;
case SpvOpTypeInt:
return inst1.GetSingleWordInOperand(0u) ==
inst2.GetSingleWordInOperand(0u) &&
inst1.GetSingleWordInOperand(1u) ==
inst2.GetSingleWordInOperand(1u);
case SpvOpTypeFloat:
case SpvOpTypePipe:
case SpvOpTypeForwardPointer:
return inst1.GetSingleWordInOperand(0u) ==
inst2.GetSingleWordInOperand(0u);
case SpvOpTypeVector:
case SpvOpTypeMatrix:
return AreTypesEqual(
*defUseManager.GetDef(inst1.GetSingleWordInOperand(0u)),
*defUseManager.GetDef(inst2.GetSingleWordInOperand(0u)),
defUseManager, decManager) &&
inst1.GetSingleWordInOperand(1u) ==
inst2.GetSingleWordInOperand(1u);
case SpvOpTypeImage:
return AreTypesEqual(
*defUseManager.GetDef(inst1.GetSingleWordInOperand(0u)),
*defUseManager.GetDef(inst2.GetSingleWordInOperand(0u)),
defUseManager, decManager) &&
inst1.GetSingleWordInOperand(1u) ==
inst2.GetSingleWordInOperand(1u) &&
inst1.GetSingleWordInOperand(2u) ==
inst2.GetSingleWordInOperand(2u) &&
inst1.GetSingleWordInOperand(3u) ==
inst2.GetSingleWordInOperand(3u) &&
inst1.GetSingleWordInOperand(4u) ==
inst2.GetSingleWordInOperand(4u) &&
inst1.GetSingleWordInOperand(5u) ==
inst2.GetSingleWordInOperand(5u) &&
inst1.GetSingleWordInOperand(6u) ==
inst2.GetSingleWordInOperand(6u) &&
inst1.NumOperands() == inst2.NumOperands() &&
(inst1.NumInOperands() == 7u ||
inst1.GetSingleWordInOperand(7u) ==
inst2.GetSingleWordInOperand(7u));
case SpvOpTypeSampledImage:
case SpvOpTypeRuntimeArray:
return AreTypesEqual(
*defUseManager.GetDef(inst1.GetSingleWordInOperand(0u)),
*defUseManager.GetDef(inst2.GetSingleWordInOperand(0u)),
defUseManager, decManager);
case SpvOpTypeArray:
return AreTypesEqual(
*defUseManager.GetDef(inst1.GetSingleWordInOperand(0u)),
*defUseManager.GetDef(inst2.GetSingleWordInOperand(0u)),
defUseManager, decManager) &&
AreTypesEqual(
*defUseManager.GetDef(inst1.GetSingleWordInOperand(1u)),
*defUseManager.GetDef(inst2.GetSingleWordInOperand(1u)),
defUseManager, decManager);
case SpvOpTypeStruct:
case SpvOpTypeFunction: {
bool res = inst1.NumInOperands() == inst2.NumInOperands();
for (uint32_t i = 0u; i < inst1.NumInOperands() && res; ++i)
res &= AreTypesEqual(
*defUseManager.GetDef(inst1.GetSingleWordInOperand(i)),
*defUseManager.GetDef(inst2.GetSingleWordInOperand(i)),
defUseManager, decManager);
return res;
}
case SpvOpTypeOpaque:
return std::strcmp(reinterpret_cast<const char*>(
inst1.GetInOperand(0u).words.data()),
reinterpret_cast<const char*>(
inst2.GetInOperand(0u).words.data())) == 0;
case SpvOpTypePointer:
return inst1.GetSingleWordInOperand(0u) ==
inst2.GetSingleWordInOperand(0u) &&
AreTypesEqual(
*defUseManager.GetDef(inst1.GetSingleWordInOperand(1u)),
*defUseManager.GetDef(inst2.GetSingleWordInOperand(1u)),
defUseManager, decManager);
default:
return false;
}
const analysis::Type* type1 =
context->get_type_mgr()->GetType(inst1.result_id());
const analysis::Type* type2 =
context->get_type_mgr()->GetType(inst2.result_id());
if (type1 && type2 && *type1 == *type2) return true;
return false;
}
} // namespace opt

3
source/opt/remove_duplicates_pass.h
View File

@ -37,8 +37,7 @@ class RemoveDuplicatesPass : public Pass {
// Returns whether two types are equal, and have the same decorations.
static bool AreTypesEqual(const ir::Instruction& inst1,
const ir::Instruction& inst2,
const analysis::DefUseManager& defUseManager,
const analysis::DecorationManager& decoManager);
ir::IRContext* context);
private:
bool RemoveDuplicateCapabilities(ir::IRContext* irContext) const;

68
source/opt/type_manager.cpp
View File

@ -40,7 +40,7 @@ TypeManager::TypeManager(const MessageConsumer& consumer,
Type* TypeManager::GetType(uint32_t id) const {
auto iter = id_to_type_.find(id);
if (iter != id_to_type_.end()) return (*iter).second.get();
if (iter != id_to_type_.end()) return (*iter).second;
return nullptr;
}
@ -67,7 +67,6 @@ ForwardPointer* TypeManager::GetForwardPointer(uint32_t index) const {
void TypeManager::AnalyzeTypes(const spvtools::ir::Module& module) {
for (const auto* inst : module.GetTypes()) RecordIfTypeDefinition(*inst);
for (const auto& inst : module.annotations()) AttachIfTypeDecoration(inst);
}
void TypeManager::RemoveId(uint32_t id) {
@ -76,22 +75,26 @@ void TypeManager::RemoveId(uint32_t id) {
auto& type = iter->second;
if (!type->IsUniqueType(true)) {
// Search for an equivalent type to re-map.
bool found = false;
for (auto& pair : id_to_type_) {
if (pair.first != id && *pair.second == *type) {
// Equivalent ambiguous type, re-map type.
type_to_id_.erase(type.get());
type_to_id_[pair.second.get()] = pair.first;
found = true;
break;
auto tIter = type_to_id_.find(type);
if (tIter != type_to_id_.end() && tIter->second == id) {
// |type| currently maps to |id|.
// Search for an equivalent type to re-map.
bool found = false;
for (auto& pair : id_to_type_) {
if (pair.first != id && *pair.second == *type) {
// Equivalent ambiguous type, re-map type.
type_to_id_.erase(type);
type_to_id_[pair.second] = pair.first;
found = true;
break;
}
}
// No equivalent ambiguous type, remove mapping.
if (!found) type_to_id_.erase(type.get());
if (!found) type_to_id_.erase(tIter);
}
} else {
// Unique type, so just erase the entry.
type_to_id_.erase(type.get());
type_to_id_.erase(type);
}
// Erase the entry for |id|.
@ -347,10 +350,15 @@ void TypeManager::CreateDecoration(uint32_t target,
}
void TypeManager::RegisterType(uint32_t id, const Type& type) {
auto& t = id_to_type_[id];
t.reset(type.Clone().release());
if (GetId(t.get()) == 0) {
type_to_id_[t.get()] = id;
// The comparison and hash on the type pool will avoid inserting the clone if
// an equivalent type already exists. The clone will be deleted when it goes
// out of scope at the end of the function in that case. Repeated insertions
// of the same Type will atmost keep one corresponding object in the type
// pool.
auto pair = type_pool_.insert(type.Clone());
id_to_type_[id] = pair.first->get();
if (GetId(pair.first->get()) == 0) {
type_to_id_[pair.first->get()] = id;
}
}
@ -482,20 +490,23 @@ Type* TypeManager::RecordIfTypeDefinition(
} else {
SPIRV_ASSERT(consumer_, type != nullptr,
"type should not be nullptr at this point");
id_to_type_[id].reset(type);
type_to_id_[type] = id;
std::vector<ir::Instruction*> decorations =
context()->get_decoration_mgr()->GetDecorationsFor(id, true);
for (auto dec : decorations) {
AttachDecoration(*dec, type);
}
std::unique_ptr<Type> unique(type);
auto pair = type_pool_.insert(std::move(unique));
id_to_type_[id] = pair.first->get();
type_to_id_[pair.first->get()] = id;
}
return type;
}
void TypeManager::AttachIfTypeDecoration(const ir::Instruction& inst) {
void TypeManager::AttachDecoration(const ir::Instruction& inst, Type* type) {
const SpvOp opcode = inst.opcode();
if (!ir::IsAnnotationInst(opcode)) return;
const uint32_t id = inst.GetSingleWordOperand(0);
// Do nothing if the id to be decorated is not for a known type.
if (!id_to_type_.count(id)) return;
Type* target_type = id_to_type_[id].get();
switch (opcode) {
case SpvOpDecorate: {
const auto count = inst.NumOperands();
@ -503,7 +514,7 @@ void TypeManager::AttachIfTypeDecoration(const ir::Instruction& inst) {
for (uint32_t i = 1; i < count; ++i) {
data.push_back(inst.GetSingleWordOperand(i));
}
target_type->AddDecoration(std::move(data));
type->AddDecoration(std::move(data));
} break;
case SpvOpMemberDecorate: {
const auto count = inst.NumOperands();
@ -512,17 +523,12 @@ void TypeManager::AttachIfTypeDecoration(const ir::Instruction& inst) {
for (uint32_t i = 2; i < count; ++i) {
data.push_back(inst.GetSingleWordOperand(i));
}
if (Struct* st = target_type->AsStruct()) {
if (Struct* st = type->AsStruct()) {
st->AddMemberDecoration(index, std::move(data));
} else {
SPIRV_UNIMPLEMENTED(consumer_, "OpMemberDecorate non-struct type");
}
} break;
case SpvOpDecorationGroup:
case SpvOpGroupDecorate:
case SpvOpGroupMemberDecorate:
SPIRV_UNIMPLEMENTED(consumer_, "unhandled decoration");
break;
default:
SPIRV_UNREACHABLE(consumer_);
break;

28
source/opt/type_manager.h
View File

@ -40,6 +40,12 @@ struct HashTypePointer {
return type->HashValue();
}
};
struct HashTypeUniquePointer {
size_t operator()(const std::unique_ptr<Type>& type) const {
assert(type);
return type->HashValue();
}
};
// Equality functor.
//
@ -52,11 +58,18 @@ struct CompareTypePointers {
return lhs->IsSame(rhs);
}
};
struct CompareTypeUniquePointers {
bool operator()(const std::unique_ptr<Type>& lhs,
const std::unique_ptr<Type>& rhs) const {
assert(lhs && rhs);
return lhs->IsSame(rhs.get());
}
};
// A class for managing the SPIR-V type hierarchy.
class TypeManager {
public:
using IdToTypeMap = std::unordered_map<uint32_t, std::unique_ptr<Type>>;
using IdToTypeMap = std::unordered_map<uint32_t, Type*>;
// Constructs a type manager from the given |module|. All internal messages
// will be communicated to the outside via the given message |consumer|.
@ -105,7 +118,7 @@ class TypeManager {
// Registers |id| to |type|.
//
// If GetId(|type|) already returns a non-zero id, the return value will be
// If GetId(|type|) already returns a non-zero id, that mapping will be
// unchanged.
void RegisterType(uint32_t id, const Type& type);
@ -120,6 +133,9 @@ class TypeManager {
private:
using TypeToIdMap = std::unordered_map<const Type*, uint32_t, HashTypePointer,
CompareTypePointers>;
using TypePool =
std::unordered_set<std::unique_ptr<Type>, HashTypeUniquePointer,
CompareTypeUniquePointers>;
using ForwardPointerVector = std::vector<std::unique_ptr<ForwardPointer>>;
// Analyzes the types and decorations on types in the given |module|.
@ -141,14 +157,16 @@ class TypeManager {
// Creates and returns a type from the given SPIR-V |inst|. Returns nullptr if
// the given instruction is not for defining a type.
Type* RecordIfTypeDefinition(const spvtools::ir::Instruction& inst);
// Attaches the decoration encoded in |inst| to a type. Does nothing if the
// given instruction is not a decoration instruction or not decorating a type.
void AttachIfTypeDecoration(const spvtools::ir::Instruction& inst);
// Attaches the decoration encoded in |inst| to |type|. Does nothing if the
// given instruction is not a decoration instruction. Assumes the target is
// |type| (e.g. should be called in loop of |type|'s decorations).
void AttachDecoration(const spvtools::ir::Instruction& inst, Type* type);
const MessageConsumer& consumer_; // Message consumer.
spvtools::ir::IRContext* context_;
IdToTypeMap id_to_type_; // Mapping from ids to their type representations.
TypeToIdMap type_to_id_; // Mapping from types to their defining ids.
TypePool type_pool_; // Memory owner of type pointers.
ForwardPointerVector forward_pointers_; // All forward pointer declarations.
// All unresolved forward pointer declarations.
// Refers the contents in the above vector.

13
test/opt/CMakeLists.txt
View File

@ -20,9 +20,9 @@ add_spvtools_unittest(TARGET instruction
)
add_spvtools_unittest(TARGET instruction_list
SRCS instruction_list_test.cpp
LIBS SPIRV-Tools-opt
)
SRCS instruction_list_test.cpp
LIBS SPIRV-Tools-opt
)
add_spvtools_unittest(TARGET ir_loader
SRCS ir_loader_test.cpp
@ -106,9 +106,9 @@ add_spvtools_unittest(TARGET pass_dead_branch_elim
)
add_spvtools_unittest(TARGET pass_dead_variable_elim
SRCS dead_variable_elim_test.cpp pass_utils.cpp
LIBS SPIRV-Tools-opt
)
SRCS dead_variable_elim_test.cpp pass_utils.cpp
LIBS SPIRV-Tools-opt
)
add_spvtools_unittest(TARGET pass_aggressive_dce
SRCS aggressive_dead_code_elim_test.cpp pass_utils.cpp
@ -255,4 +255,3 @@ add_spvtools_unittest(TARGET pass_remove_duplicates
SRCS pass_remove_duplicates_test.cpp
LIBS SPIRV-Tools-opt
)

90
test/opt/type_manager_test.cpp
View File

@ -517,6 +517,33 @@ OpMemoryModel Logical GLSL450
ASSERT_EQ(context->get_type_mgr()->GetId(&st), toStay);
}
TEST(TypeManager, RemoveIdDoesntUnmapOtherTypes) {
const std::string text = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
%1 = OpTypeInt 32 0
%2 = OpTypeStruct %1
%3 = OpTypeStruct %1
)";
std::unique_ptr<ir::IRContext> context =
BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text,
SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
EXPECT_NE(context, nullptr);
Integer u32(32, false);
Struct st({&u32});
EXPECT_EQ(1u, context->get_type_mgr()->GetId(&u32));
uint32_t id = context->get_type_mgr()->GetId(&st);
ASSERT_NE(id, 0u);
uint32_t toRemove = id == 2u ? 3u : 2u;
uint32_t toStay = id == 2u ? 2u : 3u;
context->get_type_mgr()->RemoveId(toRemove);
ASSERT_EQ(context->get_type_mgr()->GetType(toRemove), nullptr);
ASSERT_EQ(context->get_type_mgr()->GetId(&st), toStay);
}
TEST(TypeManager, GetTypeAndPointerType) {
const std::string text = R"(
OpCapability Shader
@ -552,6 +579,69 @@ OpMemoryModel Logical GLSL450
ASSERT_TRUE(pair.second->IsSame(&stPtr));
}
TEST(TypeManager, DuplicateType) {
const std::string text = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
%1 = OpTypeInt 32 0
%2 = OpTypeInt 32 0
)";
std::unique_ptr<ir::IRContext> context =
BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text,
SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
EXPECT_NE(context, nullptr);
const Type* type1 = context->get_type_mgr()->GetType(1u);
const Type* type2 = context->get_type_mgr()->GetType(2u);
EXPECT_NE(type1, nullptr);
EXPECT_NE(type2, nullptr);
EXPECT_EQ(*type1, *type2);
}
TEST(TypeManager, MultipleStructs) {
const std::string text = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
OpDecorate %3 Constant
%1 = OpTypeInt 32 0
%2 = OpTypeStruct %1
%3 = OpTypeStruct %1
)";
std::unique_ptr<ir::IRContext> context =
BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text,
SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
EXPECT_NE(context, nullptr);
const Type* type1 = context->get_type_mgr()->GetType(2u);
const Type* type2 = context->get_type_mgr()->GetType(3u);
EXPECT_NE(type1, nullptr);
EXPECT_NE(type2, nullptr);
EXPECT_FALSE(type1->IsSame(type2));
}
TEST(TypeManager, RemovingIdAvoidsUseAfterFree) {
const std::string text = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
%1 = OpTypeInt 32 0
%2 = OpTypeStruct %1
)";
std::unique_ptr<ir::IRContext> context =
BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text,
SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
EXPECT_NE(context, nullptr);
Integer u32(32, false);
Struct st({&u32});
const Type* type = context->get_type_mgr()->GetType(2u);
EXPECT_NE(type, nullptr);
context->get_type_mgr()->RemoveId(1u);
EXPECT_TRUE(type->IsSame(&st));
}
#ifdef SPIRV_EFFCEE
TEST(TypeManager, GetTypeInstructionInt) {
const std::string text = R"(

5
tools/opt/opt.cpp
View File

@ -215,6 +215,9 @@ Options (in lexicographical order):
--private-to-local
Change the scope of private variables that are used in a single
function to that function.
--remove-duplicates
Removes duplicate types, decorations, capabilities and extension
instructions.
--redundancy-elimination
Looks for instructions in the same function that compute the
same value, and deletes the redundant ones.
@ -427,6 +430,8 @@ OptStatus ParseFlags(int argc, const char** argv, Optimizer* optimizer,
optimizer->RegisterPass(CreateRedundancyEliminationPass());
} else if (0 == strcmp(cur_arg, "--private-to-local")) {
optimizer->RegisterPass(CreatePrivateToLocalPass());
} else if (0 == strcmp(cur_arg, "--remove-duplicates")) {
optimizer->RegisterPass(CreateRemoveDuplicatesPass());
} else if (0 == strcmp(cur_arg, "--relax-struct-store")) {
options->relax_struct_store = true;
} else if (0 == strcmp(cur_arg, "--skip-validation")) {