AuroraMiddleware/SPIRV-Tools
1
0
Fork 0
mirror of https://github.com/KhronosGroup/SPIRV-Tools synced 2024-11-23 12:10:06 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix erroneous uses of the type manager in copy-prop-arrays. (#1942)

Browse Source 
There are a few spots where copy propagate arrays is trying
to go from a Type to an id, but the type is not unique.  When generating
code this pass needs specific ids, otherwise we get type mismatches.
However, the ambigous types means we can sometimes get the wrong type
and generate invalid code.

That code has been rewritten to not rely on the type manager, and just
look at the instructions instead.

I have opened https://github.com/KhronosGroup/SPIRV-Tools/issues/1939 to
try to get a way to make this more robust.
This commit is contained in:
Steven Perron 2018-10-01 14:45:44 -04:00 committed by GitHub
parent fe90a1d2dc
commit 146eb3bdcf
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 344 additions and 106 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

96
source/opt/copy_prop_arrays.cpp
View File

@ -26,6 +26,8 @@ const uint32_t kLoadPointerInOperand = 0;
const uint32_t kStorePointerInOperand = 0;
const uint32_t kStoreObjectInOperand = 1;
const uint32_t kCompositeExtractObjectInOperand = 0;
const uint32_t kTypePointerStorageClassInIdx = 0;
const uint32_t kTypePointerPointeeInIdx = 1;
} // namespace
@ -51,7 +53,7 @@ Pass::Status CopyPropagateArrays::Process() {
FindSourceObjectIfPossible(&*var_inst, store_inst);
if (source_object != nullptr) {
if (CanUpdateUses(&*var_inst, source_object->GetPointerTypeId())) {
if (CanUpdateUses(&*var_inst, source_object->GetPointerTypeId(this))) {
modified = true;
PropagateObject(&*var_inst, source_object.get(), store_inst);
}
@ -139,7 +141,7 @@ Instruction* CopyPropagateArrays::BuildNewAccessChain(
return source->GetVariable();
}
return builder.AddAccessChain(source->GetPointerTypeId(),
return builder.AddAccessChain(source->GetPointerTypeId(this),
source->GetVariable()->result_id(),
source->AccessChain());
}
@ -484,14 +486,14 @@ bool CopyPropagateArrays::CanUpdateUses(Instruction* original_ptr_inst,
return true;
}
return def_use_mgr->WhileEachUse(
original_ptr_inst,
[this, type_mgr, const_mgr, type](Instruction* use, uint32_t) {
return def_use_mgr->WhileEachUse(original_ptr_inst, [this, type_mgr,
const_mgr,
type](Instruction* use,
uint32_t) {
switch (use->opcode()) {
case SpvOpLoad: {
analysis::Pointer* pointer_type = type->AsPointer();
uint32_t new_type_id =
type_mgr->GetId(pointer_type->pointee_type());
uint32_t new_type_id = type_mgr->GetId(pointer_type->pointee_type());
if (new_type_id != use->type_id()) {
return CanUpdateUses(use, new_type_id);
@ -505,8 +507,7 @@ bool CopyPropagateArrays::CanUpdateUses(Instruction* original_ptr_inst,
std::vector<uint32_t> access_chain;
for (uint32_t i = 1; i < use->NumInOperands(); ++i) {
const analysis::Constant* index_const =
const_mgr->FindDeclaredConstant(
use->GetSingleWordInOperand(i));
const_mgr->FindDeclaredConstant(use->GetSingleWordInOperand(i));
if (index_const) {
access_chain.push_back(index_const->AsIntConstant()->GetU32());
} else {
@ -544,12 +545,9 @@ bool CopyPropagateArrays::CanUpdateUses(Instruction* original_ptr_inst,
return true;
}
case SpvOpStore:
// Can't handle changing the type of a store. There are too many
// other things that might need to change as well. Not worth the
// effort. Punting for now.
// TODO (s-perron): This can be handled by expanding the store into
// a series of extracts, composite constructs, and a store.
// If needed, we can create an element-by-element copy to change the
// type of the value being stored. This way we can always handled
// stores.
return true;
case SpvOpImageTexelPointer:
case SpvOpName:
@ -579,7 +577,6 @@ void CopyPropagateArrays::UpdateUses(Instruction* original_ptr_inst,
for (auto pair : uses) {
Instruction* use = pair.first;
uint32_t index = pair.second;
analysis::Pointer* pointer_type = nullptr;
switch (use->opcode()) {
case SpvOpLoad: {
// Replace the actual use.
@ -587,8 +584,10 @@ void CopyPropagateArrays::UpdateUses(Instruction* original_ptr_inst,
use->SetOperand(index, {new_ptr_inst->result_id()});
// Update the type.
pointer_type = type_mgr->GetType(new_ptr_inst->type_id())->AsPointer();
uint32_t new_type_id = type_mgr->GetId(pointer_type->pointee_type());
Instruction* pointer_type_inst =
def_use_mgr->GetDef(new_ptr_inst->type_id());
uint32_t new_type_id =
pointer_type_inst->GetSingleWordInOperand(kTypePointerPointeeInIdx);
if (new_type_id != use->type_id()) {
use->SetResultType(new_type_id);
context()->AnalyzeUses(use);
@ -602,10 +601,6 @@ void CopyPropagateArrays::UpdateUses(Instruction* original_ptr_inst,
context()->ForgetUses(use);
use->SetOperand(index, {new_ptr_inst->result_id()});
// Update the result type.
pointer_type = type_mgr->GetType(new_ptr_inst->type_id())->AsPointer();
const analysis::Type* pointee_type = pointer_type->pointee_type();
// Convert the ids on the OpAccessChain to indices that can be used to
// get the specific member.
std::vector<uint32_t> access_chain;
@ -620,14 +615,20 @@ void CopyPropagateArrays::UpdateUses(Instruction* original_ptr_inst,
access_chain.push_back(0);
}
}
const analysis::Type* new_pointee_type =
type_mgr->GetMemberType(pointee_type, access_chain);
// Now build a pointer to the type of the member.
analysis::Pointer new_pointer_type(new_pointee_type,
pointer_type->storage_class());
Instruction* pointer_type_inst =
get_def_use_mgr()->GetDef(new_ptr_inst->type_id());
uint32_t new_pointee_type_id = GetMemberTypeId(
pointer_type_inst->GetSingleWordInOperand(kTypePointerPointeeInIdx),
access_chain);
SpvStorageClass storage_class = static_cast<SpvStorageClass>(
pointer_type_inst->GetSingleWordInOperand(
kTypePointerStorageClassInIdx));
uint32_t new_pointer_type_id =
context()->get_type_mgr()->GetTypeInstruction(&new_pointer_type);
type_mgr->FindPointerToType(new_pointee_type_id, storage_class);
if (new_pointer_type_id != use->type_id()) {
use->SetResultType(new_pointer_type_id);
@ -642,15 +643,13 @@ void CopyPropagateArrays::UpdateUses(Instruction* original_ptr_inst,
context()->ForgetUses(use);
use->SetOperand(index, {new_ptr_inst->result_id()});
uint32_t new_type_id = new_ptr_inst->type_id();
std::vector<uint32_t> access_chain;
for (uint32_t i = 1; i < use->NumInOperands(); ++i) {
access_chain.push_back(use->GetSingleWordInOperand(i));
}
const analysis::Type* type = type_mgr->GetType(new_ptr_inst->type_id());
const analysis::Type* new_type =
type_mgr->GetMemberType(type, access_chain);
uint32_t new_type_id = type_mgr->GetTypeInstruction(new_type);
new_type_id = GetMemberTypeId(new_type_id, access_chain);
if (new_type_id != use->type_id()) {
use->SetResultType(new_type_id);
@ -672,11 +671,11 @@ void CopyPropagateArrays::UpdateUses(Instruction* original_ptr_inst,
if (index == 1) {
Instruction* target_pointer = def_use_mgr->GetDef(
use->GetSingleWordInOperand(kStorePointerInOperand));
pointer_type =
type_mgr->GetType(target_pointer->type_id())->AsPointer();
uint32_t copy =
GenerateCopy(original_ptr_inst,
type_mgr->GetId(pointer_type->pointee_type()), use);
Instruction* pointer_type =
def_use_mgr->GetDef(target_pointer->type_id());
uint32_t pointee_type_id =
pointer_type->GetSingleWordInOperand(kTypePointerPointeeInIdx);
uint32_t copy = GenerateCopy(original_ptr_inst, pointee_type_id, use);
context()->ForgetUses(use);
use->SetInOperand(index, {copy});
@ -768,6 +767,29 @@ uint32_t CopyPropagateArrays::GenerateCopy(Instruction* object_inst,
return 0;
}
uint32_t CopyPropagateArrays::GetMemberTypeId(
uint32_t id, const std::vector<uint32_t>& access_chain) const {
for (uint32_t element_index : access_chain) {
Instruction* type_inst = get_def_use_mgr()->GetDef(id);
switch (type_inst->opcode()) {
case SpvOpTypeArray:
case SpvOpTypeRuntimeArray:
case SpvOpTypeMatrix:
case SpvOpTypeVector:
id = type_inst->GetSingleWordInOperand(0);
break;
case SpvOpTypeStruct:
id = type_inst->GetSingleWordInOperand(element_index);
break;
default:
break;
}
assert(id != 0 &&
"Tried to extract from an object where it cannot be done.");
}
return id;
}
void CopyPropagateArrays::MemoryObject::GetMember(
const std::vector<uint32_t>& access_chain) {
access_chain_.insert(access_chain_.end(), access_chain.begin(),

27
source/opt/copy_prop_arrays.h
View File

@ -98,18 +98,21 @@ class CopyPropagateArrays : public MemPass {
// Returns the type id of the pointer type that can be used to point to this
// memory object.
uint32_t GetPointerTypeId() const {
uint32_t GetPointerTypeId(const CopyPropagateArrays* pass) const {
analysis::DefUseManager* def_use_mgr =
GetVariable()->context()->get_def_use_mgr();
analysis::TypeManager* type_mgr =
GetVariable()->context()->get_type_mgr();
const analysis::Pointer* pointer_type =
type_mgr->GetType(GetVariable()->type_id())->AsPointer();
const analysis::Type* var_type = pointer_type->pointee_type();
const analysis::Type* member_type =
type_mgr->GetMemberType(var_type, GetAccessIds());
uint32_t member_type_id = type_mgr->GetId(member_type);
assert(member_type != 0);
Instruction* var_pointer_inst =
def_use_mgr->GetDef(GetVariable()->type_id());
uint32_t member_type_id = pass->GetMemberTypeId(
var_pointer_inst->GetSingleWordInOperand(1), GetAccessIds());
uint32_t member_pointer_type_id = type_mgr->FindPointerToType(
member_type_id, pointer_type->storage_class());
member_type_id, static_cast<SpvStorageClass>(
var_pointer_inst->GetSingleWordInOperand(0)));
return member_pointer_type_id;
}
@ -223,6 +226,12 @@ class CopyPropagateArrays : public MemPass {
// the only store that does so. Note it does not look through OpAccessChain
// instruction, so partial stores are not considered.
Instruction* FindStoreInstruction(const Instruction* var_inst) const;
// Return the type id of the member of the type |id| access using
// |access_chain|. The elements of |access_chain| are to be interpreted the
// same way the indexes are used in an |OpCompositeExtract| instruction.
uint32_t GetMemberTypeId(uint32_t id,
const std::vector<uint32_t>& access_chain) const;
};
} // namespace opt

4
source/opt/type_manager.cpp
View File

@ -59,7 +59,9 @@ std::pair<Type*, std::unique_ptr<Pointer>> TypeManager::GetTypeAndPointerType(
uint32_t TypeManager::GetId(const Type* type) const {
auto iter = type_to_id_.find(type);
if (iter != type_to_id_.end()) return (*iter).second;
if (iter != type_to_id_.end()) {
return (*iter).second;
}
return 0;
}

205
test/opt/copy_prop_array_test.cpp
View File

@ -620,6 +620,211 @@ OpFunctionEnd
SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES);
SinglePassRunAndMatch<CopyPropagateArrays>(before, false);
}
TEST_F(CopyPropArrayPassTest, IsomorphicTypes1) {
const std::string before =
R"(
; CHECK: [[int:%\w+]] = OpTypeInt 32 0
; CHECK: [[s1:%\w+]] = OpTypeStruct [[int]]
; CHECK: [[s2:%\w+]] = OpTypeStruct [[s1]]
; CHECK: [[a1:%\w+]] = OpTypeArray [[s2]]
; CHECK: [[s3:%\w+]] = OpTypeStruct [[a1]]
; CHECK: [[p_s3:%\w+]] = OpTypePointer Uniform [[s3]]
; CHECK: [[global_var:%\w+]] = OpVariable [[p_s3]] Uniform
; CHECK: [[p_a1:%\w+]] = OpTypePointer Uniform [[a1]]
; CHECK: [[p_s2:%\w+]] = OpTypePointer Uniform [[s2]]
; CHECK: [[ac1:%\w+]] = OpAccessChain [[p_a1]] [[global_var]] %uint_0
; CHECK: [[ac2:%\w+]] = OpAccessChain [[p_s2]] [[ac1]] %uint_0
; CHECK: [[ld:%\w+]] = OpLoad [[s2]] [[ac2]]
; CHECK: [[ex:%\w+]] = OpCompositeExtract [[s1]] [[ld]]
OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %2 "PS_main"
OpExecutionMode %2 OriginUpperLeft
OpSource HLSL 600
OpDecorate %3 DescriptorSet 0
OpDecorate %3 Binding 101
%uint = OpTypeInt 32 0
%uint_1 = OpConstant %uint 1
%s1 = OpTypeStruct %uint
%s2 = OpTypeStruct %s1
%a1 = OpTypeArray %s2 %uint_1
%s3 = OpTypeStruct %a1
%s1_1 = OpTypeStruct %uint
%_ptr_Uniform_uint = OpTypePointer Uniform %uint
%void = OpTypeVoid
%13 = OpTypeFunction %void
%uint_0 = OpConstant %uint 0
%s1_0 = OpTypeStruct %uint
%s2_0 = OpTypeStruct %s1_0
%a1_0 = OpTypeArray %s2_0 %uint_1
%s3_0 = OpTypeStruct %a1_0
%p_s3 = OpTypePointer Uniform %s3
%p_s3_0 = OpTypePointer Function %s3_0
%3 = OpVariable %p_s3 Uniform
%p_a1_0 = OpTypePointer Function %a1_0
%p_s2_0 = OpTypePointer Function %s2_0
%2 = OpFunction %void None %13
%20 = OpLabel
%21 = OpVariable %p_a1_0 Function
%22 = OpLoad %s3 %3
%23 = OpCompositeExtract %a1 %22 0
%24 = OpCompositeExtract %s2 %23 0
%25 = OpCompositeExtract %s1 %24 0
%26 = OpCompositeExtract %uint %25 0
%27 = OpCompositeConstruct %s1_0 %26
%32 = OpCompositeConstruct %s2_0 %27
%28 = OpCompositeConstruct %a1_0 %32
OpStore %21 %28
%29 = OpAccessChain %p_s2_0 %21 %uint_0
%30 = OpLoad %s2 %29
%31 = OpCompositeExtract %s1 %30 0
OpReturn
OpFunctionEnd
)";
SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
SetDisassembleOptions(SPV_BINARY_TO_TEXT_OPTION_NO_HEADER |
SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES);
SinglePassRunAndMatch<CopyPropagateArrays>(before, false);
}
TEST_F(CopyPropArrayPassTest, IsomorphicTypes2) {
const std::string before =
R"(
; CHECK: [[int:%\w+]] = OpTypeInt 32 0
; CHECK: [[s1:%\w+]] = OpTypeStruct [[int]]
; CHECK: [[s2:%\w+]] = OpTypeStruct [[s1]]
; CHECK: [[a1:%\w+]] = OpTypeArray [[s2]]
; CHECK: [[s3:%\w+]] = OpTypeStruct [[a1]]
; CHECK: [[p_s3:%\w+]] = OpTypePointer Uniform [[s3]]
; CHECK: [[global_var:%\w+]] = OpVariable [[p_s3]] Uniform
; CHECK: [[p_s2:%\w+]] = OpTypePointer Uniform [[s2]]
; CHECK: [[p_s1:%\w+]] = OpTypePointer Uniform [[s1]]
; CHECK: [[ac1:%\w+]] = OpAccessChain [[p_s2]] [[global_var]] %uint_0 %uint_0
; CHECK: [[ac2:%\w+]] = OpAccessChain [[p_s1]] [[ac1]] %uint_0
; CHECK: [[ld:%\w+]] = OpLoad [[s1]] [[ac2]]
; CHECK: [[ex:%\w+]] = OpCompositeExtract [[int]] [[ld]]
OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %2 "PS_main"
OpExecutionMode %2 OriginUpperLeft
OpSource HLSL 600
OpDecorate %3 DescriptorSet 0
OpDecorate %3 Binding 101
%uint = OpTypeInt 32 0
%uint_1 = OpConstant %uint 1
%_struct_6 = OpTypeStruct %uint
%_struct_7 = OpTypeStruct %_struct_6
%_arr__struct_7_uint_1 = OpTypeArray %_struct_7 %uint_1
%_struct_9 = OpTypeStruct %_arr__struct_7_uint_1
%_struct_10 = OpTypeStruct %uint
%_ptr_Uniform_uint = OpTypePointer Uniform %uint
%void = OpTypeVoid
%13 = OpTypeFunction %void
%uint_0 = OpConstant %uint 0
%_struct_15 = OpTypeStruct %uint
%_arr__struct_15_uint_1 = OpTypeArray %_struct_15 %uint_1
%_ptr_Uniform__struct_9 = OpTypePointer Uniform %_struct_9
%_ptr_Function__struct_15 = OpTypePointer Function %_struct_15
%3 = OpVariable %_ptr_Uniform__struct_9 Uniform
%_ptr_Function__arr__struct_15_uint_1 = OpTypePointer Function %_arr__struct_15_uint_1
%2 = OpFunction %void None %13
%20 = OpLabel
%21 = OpVariable %_ptr_Function__arr__struct_15_uint_1 Function
%22 = OpLoad %_struct_9 %3
%23 = OpCompositeExtract %_arr__struct_7_uint_1 %22 0
%24 = OpCompositeExtract %_struct_7 %23 0
%25 = OpCompositeExtract %_struct_6 %24 0
%26 = OpCompositeExtract %uint %25 0
%27 = OpCompositeConstruct %_struct_15 %26
%28 = OpCompositeConstruct %_arr__struct_15_uint_1 %27
OpStore %21 %28
%29 = OpAccessChain %_ptr_Function__struct_15 %21 %uint_0
%30 = OpLoad %_struct_15 %29
%31 = OpCompositeExtract %uint %30 0
OpReturn
OpFunctionEnd
)";
SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
SetDisassembleOptions(SPV_BINARY_TO_TEXT_OPTION_NO_HEADER |
SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES);
SinglePassRunAndMatch<CopyPropagateArrays>(before, false);
}
TEST_F(CopyPropArrayPassTest, IsomorphicTypes3) {
const std::string before =
R"(
; CHECK: [[int:%\w+]] = OpTypeInt 32 0
; CHECK: [[s1:%\w+]] = OpTypeStruct [[int]]
; CHECK: [[s2:%\w+]] = OpTypeStruct [[s1]]
; CHECK: [[a1:%\w+]] = OpTypeArray [[s2]]
; CHECK: [[s3:%\w+]] = OpTypeStruct [[a1]]
; CHECK: [[s1_1:%\w+]] = OpTypeStruct [[int]]
; CHECK: [[p_s3:%\w+]] = OpTypePointer Uniform [[s3]]
; CHECK: [[p_s1_1:%\w+]] = OpTypePointer Function [[s1_1]]
; CHECK: [[global_var:%\w+]] = OpVariable [[p_s3]] Uniform
; CHECK: [[p_s2:%\w+]] = OpTypePointer Uniform [[s2]]
; CHECK: [[p_s1:%\w+]] = OpTypePointer Uniform [[s1]]
; CHECK: [[var:%\w+]] = OpVariable [[p_s1_1]] Function
; CHECK: [[ac1:%\w+]] = OpAccessChain [[p_s2]] [[global_var]] %uint_0 %uint_0
; CHECK: [[ac2:%\w+]] = OpAccessChain [[p_s1]] [[ac1]] %uint_0
; CHECK: [[ld:%\w+]] = OpLoad [[s1]] [[ac2]]
; CHECK: [[ex:%\w+]] = OpCompositeExtract [[int]] [[ld]]
; CHECK: [[copy:%\w+]] = OpCompositeConstruct [[s1_1]] [[ex]]
; CHECK: OpStore [[var]] [[copy]]
OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %2 "PS_main"
OpExecutionMode %2 OriginUpperLeft
OpSource HLSL 600
OpDecorate %3 DescriptorSet 0
OpDecorate %3 Binding 101
%uint = OpTypeInt 32 0
%uint_1 = OpConstant %uint 1
%_struct_6 = OpTypeStruct %uint
%_struct_7 = OpTypeStruct %_struct_6
%_arr__struct_7_uint_1 = OpTypeArray %_struct_7 %uint_1
%_struct_9 = OpTypeStruct %_arr__struct_7_uint_1
%_ptr_Uniform_uint = OpTypePointer Uniform %uint
%void = OpTypeVoid
%13 = OpTypeFunction %void
%uint_0 = OpConstant %uint 0
%_struct_15 = OpTypeStruct %uint
%_struct_10 = OpTypeStruct %uint
%_arr__struct_15_uint_1 = OpTypeArray %_struct_15 %uint_1
%_ptr_Uniform__struct_9 = OpTypePointer Uniform %_struct_9
%_ptr_Function__struct_15 = OpTypePointer Function %_struct_15
%3 = OpVariable %_ptr_Uniform__struct_9 Uniform
%_ptr_Function__arr__struct_15_uint_1 = OpTypePointer Function %_arr__struct_15_uint_1
%2 = OpFunction %void None %13
%20 = OpLabel
%21 = OpVariable %_ptr_Function__arr__struct_15_uint_1 Function
%var = OpVariable %_ptr_Function__struct_15 Function
%22 = OpLoad %_struct_9 %3
%23 = OpCompositeExtract %_arr__struct_7_uint_1 %22 0
%24 = OpCompositeExtract %_struct_7 %23 0
%25 = OpCompositeExtract %_struct_6 %24 0
%26 = OpCompositeExtract %uint %25 0
%27 = OpCompositeConstruct %_struct_15 %26
%28 = OpCompositeConstruct %_arr__struct_15_uint_1 %27
OpStore %21 %28
%29 = OpAccessChain %_ptr_Function__struct_15 %21 %uint_0
%30 = OpLoad %_struct_15 %29
OpStore %var %30
OpReturn
OpFunctionEnd
)";
SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
SetDisassembleOptions(SPV_BINARY_TO_TEXT_OPTION_NO_HEADER |
SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES);
SinglePassRunAndMatch<CopyPropagateArrays>(before, false);
}
#endif // SPIRV_EFFCEE
// This test will place a load before the store. We cannot propagate in this