Copy decorations when creating new ids. (#1843)

* Copy decorations when creating new ids.

When creating a new value based on an old value, we need to copy the
decorations to the new id.  This change does this in 3 places:

1) The variable holding the return value of the function generated by
merge return should get decorations from the function.

2) The results of the OpPhi instructions should get decorations from the
variable they are replacing in the ssa writer.

3) In local access chain convert the intermediate struct (result of
OpCompositeInsert) generated for the store replacement should get its
decorations from the variable being stored to.

Fixes #1787.
This commit is contained in:
Steven Perron 2018-08-24 11:55:39 -04:00 committed by GitHub
parent 6d27a8350f
commit d746681fe9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 455 additions and 7 deletions

View File

@ -400,6 +400,47 @@ void DecorationManager::CloneDecorations(uint32_t from, uint32_t to) {
}
}
void DecorationManager::CloneDecorations(
uint32_t from, uint32_t to,
const std::vector<SpvDecoration>& decorations_to_copy) {
const auto decoration_list = id_to_decoration_insts_.find(from);
if (decoration_list == id_to_decoration_insts_.end()) return;
auto context = module_->context();
for (Instruction* inst : decoration_list->second.direct_decorations) {
if (std::find(decorations_to_copy.begin(), decorations_to_copy.end(),
inst->GetSingleWordInOperand(1)) ==
decorations_to_copy.end()) {
continue;
}
// Clone decoration and change |target-id| to |to|.
std::unique_ptr<Instruction> new_inst(inst->Clone(module_->context()));
new_inst->SetInOperand(0, {to});
module_->AddAnnotationInst(std::move(new_inst));
auto decoration_iter = --module_->annotation_end();
context->AnalyzeUses(&*decoration_iter);
}
// We need to copy the list of instructions as ForgetUses and AnalyzeUses are
// going to modify it.
std::vector<Instruction*> indirect_decorations =
decoration_list->second.indirect_decorations;
for (Instruction* inst : indirect_decorations) {
switch (inst->opcode()) {
case SpvOpGroupDecorate:
CloneDecorations(inst->GetSingleWordInOperand(0), to,
decorations_to_copy);
break;
case SpvOpGroupMemberDecorate: {
assert(false && "The source id is not suppose to be a type.");
break;
}
default:
assert(false && "Unexpected decoration instruction");
}
}
}
void DecorationManager::RemoveDecoration(Instruction* inst) {
const auto remove_from_container = [inst](std::vector<Instruction*>& v) {
v.erase(std::remove(v.begin(), v.end(), inst), v.end());
@ -433,7 +474,6 @@ void DecorationManager::RemoveDecoration(Instruction* inst) {
break;
}
}
} // namespace analysis
} // namespace opt
} // namespace spvtools

View File

@ -91,6 +91,12 @@ class DecorationManager {
// This function does not check if the id |to| is already decorated.
void CloneDecorations(uint32_t from, uint32_t to);
// Same as above, but only clone the decoration if the decoration operand is
// in |decorations_to_copy|. This function has the extra restriction that
// |from| and |to| must not be an object, not a type.
void CloneDecorations(uint32_t from, uint32_t to,
const std::vector<SpvDecoration>& decorations_to_copy);
// Informs the decoration manager of a new decoration that it needs to track.
void AddDecoration(Instruction* inst);

View File

@ -78,6 +78,8 @@ void LocalAccessChainConvertPass::ReplaceAccessChainLoad(
uint32_t varPteTypeId;
const uint32_t ldResultId =
BuildAndAppendVarLoad(address_inst, &varId, &varPteTypeId, &new_inst);
context()->get_decoration_mgr()->CloneDecorations(
original_load->result_id(), ldResultId, {SpvDecorationRelaxedPrecision});
original_load->InsertBefore(std::move(new_inst));
// Rewrite |original_load| into an extract.
@ -103,6 +105,8 @@ void LocalAccessChainConvertPass::GenAccessChainStoreReplacement(
uint32_t varPteTypeId;
const uint32_t ldResultId =
BuildAndAppendVarLoad(ptrInst, &varId, &varPteTypeId, newInsts);
context()->get_decoration_mgr()->CloneDecorations(
varId, ldResultId, {SpvDecorationRelaxedPrecision});
// Build and append Insert
const uint32_t insResultId = TakeNextId();
@ -113,6 +117,9 @@ void LocalAccessChainConvertPass::GenAccessChainStoreReplacement(
BuildAndAppendInst(SpvOpCompositeInsert, varPteTypeId, insResultId,
ins_in_opnds, newInsts);
context()->get_decoration_mgr()->CloneDecorations(
varId, insResultId, {SpvDecorationRelaxedPrecision});
// Build and append Store
BuildAndAppendInst(SpvOpStore, 0, 0,
{{spv_operand_type_t::SPV_OPERAND_TYPE_ID, {varId}},

View File

@ -150,6 +150,8 @@ void MergeReturnPass::CreateReturn(BasicBlock* block) {
Instruction* var_inst = block->terminator();
context()->AnalyzeDefUse(var_inst);
context()->set_instr_block(var_inst, block);
context()->get_decoration_mgr()->CloneDecorations(
return_value_->result_id(), loadId, {SpvDecorationRelaxedPrecision});
block->AddInstruction(MakeUnique<Instruction>(
context(), SpvOpReturnValue, 0, 0,
@ -669,6 +671,9 @@ void MergeReturnPass::AddReturnValue() {
return_value_ = &*entry_block->begin();
context()->AnalyzeDefUse(return_value_);
context()->set_instr_block(return_value_, entry_block);
context()->get_decoration_mgr()->CloneDecorations(
function_->result_id(), var_id, {SpvDecorationRelaxedPrecision});
}
void MergeReturnPass::AddReturnFlag() {

View File

@ -452,6 +452,11 @@ bool SSARewriter::ApplyReplacements() {
pass_->context()->set_instr_block(&*phi_inst, phi_candidate->bb());
auto insert_it = phi_candidate->bb()->begin();
insert_it.InsertBefore(std::move(phi_inst));
pass_->context()->get_decoration_mgr()->CloneDecorations(
phi_candidate->var_id(), phi_candidate->result_id(),
{SpvDecorationRelaxedPrecision});
modified = true;
}

View File

@ -534,8 +534,7 @@ OpGroupDecorate %3 %1
)";
DecorationManager* decoManager = GetDecorationManager(spirv);
EXPECT_THAT(GetErrorMessage(), "");
decoManager->RemoveDecorationsFrom(
1u, [](const Instruction& inst) {
decoManager->RemoveDecorationsFrom(1u, [](const Instruction& inst) {
return inst.opcode() == SpvOpDecorate &&
inst.GetSingleWordInOperand(0u) == 3u &&
inst.GetSingleWordInOperand(1u) == SpvDecorationBuiltIn;
@ -735,6 +734,123 @@ OpDecorate %5 Aliased
EXPECT_THAT(ModuleToText(), expected_binary);
}
TEST_F(DecorationManagerTest, CloneSomeDecorations) {
const std::string spirv = R"(OpCapability Shader
OpCapability Linkage
OpExtension "SPV_GOOGLE_hlsl_functionality1"
OpExtension "SPV_GOOGLE_decorate_string"
OpMemoryModel Logical GLSL450
OpDecorate %1 RelaxedPrecision
OpDecorate %1 Restrict
%2 = OpTypeInt 32 0
%3 = OpTypePointer Function %2
%4 = OpTypeVoid
%5 = OpTypeFunction %4
%6 = OpFunction %4 None %5
%7 = OpLabel
%1 = OpVariable %3 Function
%8 = OpUndef %2
OpReturn
OpFunctionEnd
)";
DecorationManager* decoManager = GetDecorationManager(spirv);
EXPECT_EQ(GetErrorMessage(), "");
// Check cloning OpDecorate including group decorations.
auto decorations = decoManager->GetDecorationsFor(8u, false);
EXPECT_EQ(GetErrorMessage(), "");
EXPECT_TRUE(decorations.empty());
decoManager->CloneDecorations(1u, 8u, {SpvDecorationRelaxedPrecision});
decorations = decoManager->GetDecorationsFor(8u, false);
EXPECT_THAT(GetErrorMessage(), "");
std::string expected_decorations =
R"(OpDecorate %8 RelaxedPrecision
)";
EXPECT_EQ(ToText(decorations), expected_decorations);
const std::string expected_binary = R"(OpCapability Shader
OpCapability Linkage
OpExtension "SPV_GOOGLE_hlsl_functionality1"
OpExtension "SPV_GOOGLE_decorate_string"
OpMemoryModel Logical GLSL450
OpDecorate %1 RelaxedPrecision
OpDecorate %1 Restrict
OpDecorate %8 RelaxedPrecision
%2 = OpTypeInt 32 0
%3 = OpTypePointer Function %2
%4 = OpTypeVoid
%5 = OpTypeFunction %4
%6 = OpFunction %4 None %5
%7 = OpLabel
%1 = OpVariable %3 Function
%8 = OpUndef %2
OpReturn
OpFunctionEnd
)";
EXPECT_EQ(ModuleToText(), expected_binary);
}
// Test cloning decoration for an id that is decorated via a group decoration.
TEST_F(DecorationManagerTest, CloneSomeGroupDecorations) {
const std::string spirv = R"(OpCapability Shader
OpCapability Linkage
OpMemoryModel Logical GLSL450
OpDecorate %1 RelaxedPrecision
OpDecorate %1 Restrict
%1 = OpDecorationGroup
OpGroupDecorate %1 %2
%3 = OpTypeInt 32 0
%4 = OpTypePointer Function %3
%5 = OpTypeVoid
%6 = OpTypeFunction %5
%7 = OpFunction %5 None %6
%8 = OpLabel
%2 = OpVariable %4 Function
%9 = OpUndef %3
OpReturn
OpFunctionEnd
)";
DecorationManager* decoManager = GetDecorationManager(spirv);
EXPECT_EQ(GetErrorMessage(), "");
// Check cloning OpDecorate including group decorations.
auto decorations = decoManager->GetDecorationsFor(9u, false);
EXPECT_EQ(GetErrorMessage(), "");
EXPECT_TRUE(decorations.empty());
decoManager->CloneDecorations(2u, 9u, {SpvDecorationRelaxedPrecision});
decorations = decoManager->GetDecorationsFor(9u, false);
EXPECT_THAT(GetErrorMessage(), "");
std::string expected_decorations =
R"(OpDecorate %9 RelaxedPrecision
)";
EXPECT_EQ(ToText(decorations), expected_decorations);
const std::string expected_binary = R"(OpCapability Shader
OpCapability Linkage
OpMemoryModel Logical GLSL450
OpDecorate %1 RelaxedPrecision
OpDecorate %1 Restrict
%1 = OpDecorationGroup
OpGroupDecorate %1 %2
OpDecorate %9 RelaxedPrecision
%3 = OpTypeInt 32 0
%4 = OpTypePointer Function %3
%5 = OpTypeVoid
%6 = OpTypeFunction %5
%7 = OpFunction %5 None %6
%8 = OpLabel
%2 = OpVariable %4 Function
%9 = OpUndef %3
OpReturn
OpFunctionEnd
)";
EXPECT_EQ(ModuleToText(), expected_binary);
}
TEST_F(DecorationManagerTest, HaveTheSameDecorationsWithoutGroupsTrue) {
const std::string spirv = R"(
OpCapability Shader

View File

@ -471,6 +471,160 @@ OpFunctionEnd
SinglePassRunAndMatch<LocalAccessChainConvertPass>(predefs + before, true);
}
TEST_F(LocalAccessChainConvertTest,
StructOfVecsOfFloatConvertedWithDecorationOnLoad) {
// #version 140
//
// in vec4 BaseColor;
//
// struct S_t {
// vec4 v0;
// vec4 v1;
// };
//
// void main()
// {
// S_t s0;
// s0.v1 = BaseColor;
// gl_FragColor = s0.v1;
// }
const std::string predefs_before =
R"(OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %main "main" %BaseColor %gl_FragColor
OpExecutionMode %main OriginUpperLeft
OpSource GLSL 140
OpName %main "main"
OpName %S_t "S_t"
OpMemberName %S_t 0 "v0"
OpMemberName %S_t 1 "v1"
OpName %s0 "s0"
OpName %BaseColor "BaseColor"
OpName %gl_FragColor "gl_FragColor"
OpDecorate %21 RelaxedPrecision
%void = OpTypeVoid
%8 = OpTypeFunction %void
%float = OpTypeFloat 32
%v4float = OpTypeVector %float 4
%S_t = OpTypeStruct %v4float %v4float
%_ptr_Function_S_t = OpTypePointer Function %S_t
%int = OpTypeInt 32 1
%int_1 = OpConstant %int 1
%_ptr_Input_v4float = OpTypePointer Input %v4float
%BaseColor = OpVariable %_ptr_Input_v4float Input
%_ptr_Function_v4float = OpTypePointer Function %v4float
%_ptr_Output_v4float = OpTypePointer Output %v4float
%gl_FragColor = OpVariable %_ptr_Output_v4float Output
)";
const std::string before =
R"(
; CHECK: OpDecorate
; CHECK: OpDecorate [[ld2:%\w+]] RelaxedPrecision
; CHECK-NOT: OpDecorate
; CHECK: [[st_id:%\w+]] = OpLoad %v4float %BaseColor
; CHECK: [[ld1:%\w+]] = OpLoad %S_t %s0
; CHECK: [[ins:%\w+]] = OpCompositeInsert %S_t [[st_id]] [[ld1]] 1
; CHECK: OpStore %s0 [[ins]]
; CHECK: [[ld2]] = OpLoad %S_t %s0
; CHECK: [[ex2:%\w+]] = OpCompositeExtract %v4float [[ld2]] 1
; CHECK: OpStore %gl_FragColor [[ex2]]
%main = OpFunction %void None %8
%17 = OpLabel
%s0 = OpVariable %_ptr_Function_S_t Function
%18 = OpLoad %v4float %BaseColor
%19 = OpAccessChain %_ptr_Function_v4float %s0 %int_1
OpStore %19 %18
%20 = OpAccessChain %_ptr_Function_v4float %s0 %int_1
%21 = OpLoad %v4float %20
OpStore %gl_FragColor %21
OpReturn
OpFunctionEnd
)";
SinglePassRunAndMatch<LocalAccessChainConvertPass>(predefs_before + before,
true);
}
TEST_F(LocalAccessChainConvertTest,
StructOfVecsOfFloatConvertedWithDecorationOnStore) {
// #version 140
//
// in vec4 BaseColor;
//
// struct S_t {
// vec4 v0;
// vec4 v1;
// };
//
// void main()
// {
// S_t s0;
// s0.v1 = BaseColor;
// gl_FragColor = s0.v1;
// }
const std::string predefs_before =
R"(OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %main "main" %BaseColor %gl_FragColor
OpExecutionMode %main OriginUpperLeft
OpSource GLSL 140
OpName %main "main"
OpName %S_t "S_t"
OpMemberName %S_t 0 "v0"
OpMemberName %S_t 1 "v1"
OpName %s0 "s0"
OpName %BaseColor "BaseColor"
OpName %gl_FragColor "gl_FragColor"
OpDecorate %s0 RelaxedPrecision
%void = OpTypeVoid
%8 = OpTypeFunction %void
%float = OpTypeFloat 32
%v4float = OpTypeVector %float 4
%S_t = OpTypeStruct %v4float %v4float
%_ptr_Function_S_t = OpTypePointer Function %S_t
%int = OpTypeInt 32 1
%int_1 = OpConstant %int 1
%_ptr_Input_v4float = OpTypePointer Input %v4float
%BaseColor = OpVariable %_ptr_Input_v4float Input
%_ptr_Function_v4float = OpTypePointer Function %v4float
%_ptr_Output_v4float = OpTypePointer Output %v4float
%gl_FragColor = OpVariable %_ptr_Output_v4float Output
)";
const std::string before =
R"(
; CHECK: OpDecorate
; CHECK: OpDecorate [[ld1:%\w+]] RelaxedPrecision
; CHECK: OpDecorate [[ins:%\w+]] RelaxedPrecision
; CHECK-NOT: OpDecorate
; CHECK: [[st_id:%\w+]] = OpLoad %v4float %BaseColor
; CHECK: [[ld1]] = OpLoad %S_t %s0
; CHECK: [[ins]] = OpCompositeInsert %S_t [[st_id]] [[ld1]] 1
; CHECK: OpStore %s0 [[ins]]
; CHECK: [[ld2:%\w+]] = OpLoad %S_t %s0
; CHECK: [[ex2:%\w+]] = OpCompositeExtract %v4float [[ld2]] 1
; CHECK: OpStore %gl_FragColor [[ex2]]
%main = OpFunction %void None %8
%17 = OpLabel
%s0 = OpVariable %_ptr_Function_S_t Function
%18 = OpLoad %v4float %BaseColor
%19 = OpAccessChain %_ptr_Function_v4float %s0 %int_1
OpStore %19 %18
%20 = OpAccessChain %_ptr_Function_v4float %s0 %int_1
%21 = OpLoad %v4float %20
OpStore %gl_FragColor %21
OpReturn
OpFunctionEnd
)";
SinglePassRunAndMatch<LocalAccessChainConvertPass>(predefs_before + before,
true);
}
#endif // SPIRV_EFFCEE
TEST_F(LocalAccessChainConvertTest, DynamicallyIndexedVarNotConverted) {

View File

@ -1719,6 +1719,45 @@ TEST_F(LocalSSAElimTest, CompositeExtractProblem) {
SinglePassRunAndMatch<SSARewritePass>(spv_asm, true);
}
// Test that the RelaxedPrecision decoration on the variable to added to the
// result of the OpPhi instruction.
TEST_F(LocalSSAElimTest, DecoratedVariable) {
const std::string spv_asm = R"(
; CHECK: OpDecorate [[var:%\w+]] RelaxedPrecision
; CHECK: OpDecorate [[phi_id:%\w+]] RelaxedPrecision
; CHECK: [[phi_id]] = OpPhi
OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %2 "main"
OpDecorate %v RelaxedPrecision
%void = OpTypeVoid
%func_t = OpTypeFunction %void
%bool = OpTypeBool
%true = OpConstantTrue %bool
%int = OpTypeInt 32 0
%int_p = OpTypePointer Function %int
%int_1 = OpConstant %int 1
%int_0 = OpConstant %int 0
%2 = OpFunction %void None %func_t
%33 = OpLabel
%v = OpVariable %int_p Function
OpSelectionMerge %merge None
OpBranchConditional %true %l1 %l2
%l1 = OpLabel
OpStore %v %int_1
OpBranch %merge
%l2 = OpLabel
OpStore %v %int_0
OpBranch %merge
%merge = OpLabel
%ld = OpLoad %int %v
OpReturn
OpFunctionEnd)";
SinglePassRunAndMatch<SSARewritePass>(spv_asm, true);
}
#endif
// TODO(greg-lunarg): Add tests to verify handling of these cases:

View File

@ -1078,6 +1078,82 @@ OpFunctionEnd
SinglePassRunAndCheck<MergeReturnPass>(before, after, false, true);
}
TEST_F(MergeReturnPassTest, ReturnValueDecoration) {
const std::string before =
R"(OpCapability Linkage
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %11 "simple_shader"
OpDecorate %7 RelaxedPrecision
%12 = OpTypeVoid
%1 = OpTypeInt 32 0
%2 = OpTypeBool
%3 = OpConstantFalse %2
%4 = OpConstant %1 0
%5 = OpConstant %1 1
%6 = OpTypeFunction %1
%13 = OpTypeFunction %12
%11 = OpFunction %12 None %13
%l1 = OpLabel
OpReturn
OpFunctionEnd
%7 = OpFunction %1 None %6
%8 = OpLabel
OpBranchConditional %3 %9 %10
%9 = OpLabel
OpReturnValue %4
%10 = OpLabel
OpReturnValue %5
OpFunctionEnd
)";
const std::string after =
R"(OpCapability Linkage
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %11 "simple_shader"
OpDecorate %7 RelaxedPrecision
OpDecorate %17 RelaxedPrecision
OpDecorate %18 RelaxedPrecision
%12 = OpTypeVoid
%1 = OpTypeInt 32 0
%2 = OpTypeBool
%3 = OpConstantFalse %2
%4 = OpConstant %1 0
%5 = OpConstant %1 1
%6 = OpTypeFunction %1
%13 = OpTypeFunction %12
%16 = OpTypePointer Function %1
%19 = OpTypePointer Function %2
%21 = OpConstantTrue %2
%11 = OpFunction %12 None %13
%14 = OpLabel
OpReturn
OpFunctionEnd
%7 = OpFunction %1 None %6
%8 = OpLabel
%20 = OpVariable %19 Function %3
%17 = OpVariable %16 Function
OpBranchConditional %3 %9 %10
%9 = OpLabel
OpStore %20 %21
OpStore %17 %4
OpBranch %15
%10 = OpLabel
OpStore %20 %21
OpStore %17 %5
OpBranch %15
%15 = OpLabel
%18 = OpLoad %1 %17
OpReturnValue %18
OpFunctionEnd
)";
SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
SetDisassembleOptions(SPV_BINARY_TO_TEXT_OPTION_NO_HEADER);
SinglePassRunAndCheck<MergeReturnPass>(before, after, false, true);
}
} // namespace
} // namespace opt
} // namespace spvtools