Simplifying code for adding instructions to worklist
* AddToWorklist can now be called unconditionally
* It will only add instructions that have not already been marked as
live
* Fixes a case where a merge was not added to the worklist because the
branch was already marked as live
* Added two similar tests that fail without the fix
diff --git a/source/opt/aggressive_dead_code_elim_pass.cpp b/source/opt/aggressive_dead_code_elim_pass.cpp
index 2e79fe6..8125d57 100644
--- a/source/opt/aggressive_dead_code_elim_pass.cpp
+++ b/source/opt/aggressive_dead_code_elim_pass.cpp
@@ -117,7 +117,7 @@
// If default, assume it stores e.g. frexp, modf, function call
case SpvOpStore:
default:
- if (!IsLive(user)) AddToWorklist(user);
+ AddToWorklist(user);
break;
}
});
@@ -251,12 +251,10 @@
if (hdrMerge == loopMerge) break;
branchInst = hdrBranch;
}
- if (!IsLive(user)) {
- AddToWorklist(user);
- // Add branch's merge if there is one
- ir::Instruction* userMerge = branch2merge_[user];
- if (userMerge != nullptr) AddToWorklist(userMerge);
- }
+ AddToWorklist(user);
+ // Add branch's merge if there is one
+ ir::Instruction* userMerge = branch2merge_[user];
+ if (userMerge != nullptr) AddToWorklist(userMerge);
});
const uint32_t contId =
loopMerge->GetSingleWordInOperand(kLoopMergeContinueBlockIdInIdx);
@@ -272,7 +270,7 @@
hdrMerge->GetSingleWordInOperand(kSelectionMergeMergeBlockIdInIdx);
if (hdrMergeId == contId) return;
// Need to mark merge instruction too
- if (!IsLive(hdrMerge)) AddToWorklist(hdrMerge);
+ AddToWorklist(hdrMerge);
}
} else if (op == SpvOpBranch) {
// An unconditional branch can only be a continue if it is not
@@ -288,7 +286,7 @@
} else {
return;
}
- if (!IsLive(user)) AddToWorklist(user);
+ AddToWorklist(user);
});
}
@@ -392,7 +390,7 @@
// as part of live code discovery and can create false live code,
// for example, the branch to a header of a loop.
if (inInst->opcode() == SpvOpLabel && liveInst->IsBranch()) return;
- if (!IsLive(inInst)) AddToWorklist(inInst);
+ AddToWorklist(inInst);
});
if (liveInst->type_id() != 0) {
AddToWorklist(get_def_use_mgr()->GetDef(liveInst->type_id()));
@@ -403,7 +401,7 @@
// worklist.
ir::BasicBlock* blk = context()->get_instr_block(liveInst);
ir::Instruction* branchInst = block2headerBranch_[blk];
- if (branchInst != nullptr && !IsLive(branchInst)) {
+ if (branchInst != nullptr) {
AddToWorklist(branchInst);
ir::Instruction* mergeInst = branch2merge_[branchInst];
AddToWorklist(mergeInst);
diff --git a/source/opt/aggressive_dead_code_elim_pass.h b/source/opt/aggressive_dead_code_elim_pass.h
index 3669752..e3ae3bb 100644
--- a/source/opt/aggressive_dead_code_elim_pass.h
+++ b/source/opt/aggressive_dead_code_elim_pass.h
@@ -72,8 +72,7 @@
// Add |inst| to worklist_ and live_insts_.
void AddToWorklist(ir::Instruction* inst) {
- live_insts_.insert(inst);
- worklist_.push(inst);
+ if (live_insts_.insert(inst).second) worklist_.push(inst);
}
// Add all store instruction which use |ptrId|, directly or indirectly,
diff --git a/test/opt/aggressive_dead_code_elim_test.cpp b/test/opt/aggressive_dead_code_elim_test.cpp
index dd079d9..b79eed3 100644
--- a/test/opt/aggressive_dead_code_elim_test.cpp
+++ b/test/opt/aggressive_dead_code_elim_test.cpp
@@ -5176,6 +5176,7 @@
SinglePassRunAndMatch<opt::AggressiveDCEPass>(text, true);
}
+#endif // SPIRV_EFFCEE
TEST_F(AggressiveDCETest, BreaksDontVisitPhis) {
const std::string text = R"(
@@ -5219,7 +5220,84 @@
std::get<1>(SinglePassRunAndDisassemble<opt::AggressiveDCEPass>(
text, false, true)));
}
-#endif // SPIRV_EFFCEE
+
+// Test for #1212
+TEST_F(AggressiveDCETest, ConstStoreInnerLoop) {
+ const std::string text = R"(OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Vertex %1 "main" %2
+%void = OpTypeVoid
+%4 = OpTypeFunction %void
+%float = OpTypeFloat 32
+%bool = OpTypeBool
+%true = OpConstantTrue %bool
+%_ptr_Output_float = OpTypePointer Output %float
+%2 = OpVariable %_ptr_Output_float Output
+%float_3 = OpConstant %float 3
+%1 = OpFunction %void None %4
+%13 = OpLabel
+OpBranch %14
+%14 = OpLabel
+OpLoopMerge %15 %16 None
+OpBranchConditional %true %17 %15
+%17 = OpLabel
+OpStore %2 %float_3
+OpLoopMerge %18 %17 None
+OpBranchConditional %true %18 %17
+%18 = OpLabel
+OpBranch %15
+%16 = OpLabel
+OpBranch %14
+%15 = OpLabel
+OpBranch %20
+%20 = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+ SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+ SinglePassRunAndCheck<opt::AggressiveDCEPass>(text, text, true, true);
+}
+
+// Test for #1212
+TEST_F(AggressiveDCETest, InnerLoopCopy) {
+ const std::string text = R"(OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Vertex %1 "main" %2 %3
+%void = OpTypeVoid
+%5 = OpTypeFunction %void
+%float = OpTypeFloat 32
+%bool = OpTypeBool
+%true = OpConstantTrue %bool
+%_ptr_Output_float = OpTypePointer Output %float
+%_ptr_Input_float = OpTypePointer Input %float
+%2 = OpVariable %_ptr_Output_float Output
+%3 = OpVariable %_ptr_Input_float Input
+%1 = OpFunction %void None %5
+%14 = OpLabel
+OpBranch %15
+%15 = OpLabel
+OpLoopMerge %16 %17 None
+OpBranchConditional %true %18 %16
+%18 = OpLabel
+%19 = OpLoad %float %3
+OpStore %2 %19
+OpLoopMerge %20 %18 None
+OpBranchConditional %true %20 %18
+%20 = OpLabel
+OpBranch %16
+%17 = OpLabel
+OpBranch %15
+%16 = OpLabel
+OpBranch %22
+%22 = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+ SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+ SinglePassRunAndCheck<opt::AggressiveDCEPass>(text, text, true, true);
+}
// TODO(greg-lunarg): Add tests to verify handling of these cases:
//
