Prevent unnecessary changes to the IR in dead branch elim

* When handling unreachable merges and continues, do not optimize to the
same IR
 * pass did not check whether the unreachable blocks were in the
 optimized form before transforming them
* added a test to catch this issue
This commit is contained in:
Alan Baker 2018-01-30 15:00:27 -05:00 committed by David Neto
parent c86cb76a22
commit 16949236fe
2 changed files with 74 additions and 27 deletions

View File

@ -216,16 +216,25 @@ bool DeadBranchElimPass::FixPhiNodesInLiveBlocks(
auto cont_iter = unreachable_continues.find(inc);
if (cont_iter != unreachable_continues.end() &&
cont_iter->second == &block) {
// Replace incoming value with undef if this phi exists in the loop
// header. Otherwise, this edge is not live since the unreachable
// continue block will be replaced with an unconditional branch to
// the header only.
operands.emplace_back(
SPV_OPERAND_TYPE_ID,
std::initializer_list<uint32_t>{Type2Undef(inst->type_id())});
operands.push_back(inst->GetInOperand(i));
changed = true;
backedge_added = true;
if (get_def_use_mgr()
->GetDef(inst->GetSingleWordInOperand(i - 1))
->opcode() == SpvOpUndef) {
// Already undef incoming value, no change necessary.
operands.push_back(inst->GetInOperand(i - 1));
operands.push_back(inst->GetInOperand(i));
backedge_added = true;
} else {
// Replace incoming value with undef if this phi exists in the
// loop header. Otherwise, this edge is not live since the
// unreachable continue block will be replaced with an
// unconditional branch to the header only.
operands.emplace_back(
SPV_OPERAND_TYPE_ID,
std::initializer_list<uint32_t>{Type2Undef(inst->type_id())});
operands.push_back(inst->GetInOperand(i));
changed = true;
backedge_added = true;
}
} else if (live_blocks.count(inc) && inc->IsSuccessor(&block)) {
// Keep live incoming edge.
operands.push_back(inst->GetInOperand(i - 1));
@ -237,6 +246,7 @@ bool DeadBranchElimPass::FixPhiNodesInLiveBlocks(
}
if (changed) {
modified = true;
uint32_t continue_id = block.ContinueBlockIdIfAny();
if (!backedge_added && continue_id != 0 &&
unreachable_continues.count(GetParentBlock(continue_id))) {
@ -291,27 +301,34 @@ bool DeadBranchElimPass::EraseDeadBlocks(
bool modified = false;
for (auto ebi = func->begin(); ebi != func->end();) {
if (unreachable_merges.count(&*ebi)) {
// Make unreachable, but leave the label.
KillAllInsts(&*ebi, false);
// Add unreachable terminator.
ebi->AddInstruction(
MakeUnique<ir::Instruction>(context(), SpvOpUnreachable, 0, 0,
std::initializer_list<ir::Operand>{}));
if (ebi->begin() != ebi->tail() ||
ebi->terminator()->opcode() != SpvOpUnreachable) {
// Make unreachable, but leave the label.
KillAllInsts(&*ebi, false);
// Add unreachable terminator.
ebi->AddInstruction(
MakeUnique<ir::Instruction>(context(), SpvOpUnreachable, 0, 0,
std::initializer_list<ir::Operand>{}));
modified = true;
}
++ebi;
modified = true;
} else if (unreachable_continues.count(&*ebi)) {
// Make unreachable, but leave the label.
KillAllInsts(&*ebi, false);
// Add unconditional branch to header.
assert(unreachable_continues.count(&*ebi));
uint32_t cont_id = unreachable_continues.find(&*ebi)->second->id();
ebi->AddInstruction(
MakeUnique<ir::Instruction>(context(), SpvOpBranch, 0, 0,
std::initializer_list<ir::Operand>{
{SPV_OPERAND_TYPE_ID, {cont_id}}}));
get_def_use_mgr()->AnalyzeInstUse(&*ebi->tail());
if (ebi->begin() != ebi->tail() ||
ebi->terminator()->opcode() != SpvOpBranch ||
ebi->terminator()->GetSingleWordInOperand(0u) != cont_id) {
// Make unreachable, but leave the label.
KillAllInsts(&*ebi, false);
// Add unconditional branch to header.
assert(unreachable_continues.count(&*ebi));
ebi->AddInstruction(
MakeUnique<ir::Instruction>(context(), SpvOpBranch, 0, 0,
std::initializer_list<ir::Operand>{
{SPV_OPERAND_TYPE_ID, {cont_id}}}));
get_def_use_mgr()->AnalyzeInstUse(&*ebi->tail());
modified = true;
}
++ebi;
modified = true;
} else if (!live_blocks.count(&*ebi)) {
// Kill this block.
KillAllInsts(&*ebi);

View File

@ -1763,6 +1763,36 @@ OpFunctionEnd
SinglePassRunAndMatch<opt::DeadBranchElimPass>(text, true);
}
TEST_F(DeadBranchElimTest, NoUnnecessaryChanges) {
const std::string text = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %func "func"
%void = OpTypeVoid
%bool = OpTypeBool
%true = OpConstantTrue %bool
%undef = OpUndef %bool
%functy = OpTypeFunction %void
%func = OpFunction %void None %functy
%1 = OpLabel
OpBranch %2
%2 = OpLabel
%3 = OpPhi %bool %true %1 %undef %5
OpLoopMerge %4 %5 None
OpBranch %6
%6 = OpLabel
OpReturn
%5 = OpLabel
OpBranch %2
%4 = OpLabel
OpUnreachable
OpFunctionEnd
)";
auto result = SinglePassRunToBinary<opt::DeadBranchElimPass>(text, true);
EXPECT_EQ(std::get<1>(result), opt::Pass::Status::SuccessWithoutChange);
}
TEST_F(DeadBranchElimTest, ExtraBackedgePartiallyDead) {
const std::string text = R"(
; CHECK: OpLabel