SSA rewriter: Don't use trivial phis (#2757)

When a phi candidate is marked as trivial, we are suppose to update all
of its uses to the reference the value that it is being folded to.
However, the code updates the uses misses `defs_at_block_`.  So at a
later time, the id for the trivial phi can reemerge.

Fixes #2744
This commit is contained in:
Steven Perron 2019-07-23 17:59:30 -04:00 committed by GitHub
parent aea4e6b1b9
commit c9190a54da
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 67 additions and 0 deletions

View File

@ -102,6 +102,7 @@ void SSARewriter::ReplacePhiUsersWith(const PhiCandidate& phi_to_remove,
uint32_t repl_id) {
for (uint32_t user_id : phi_to_remove.users()) {
PhiCandidate* user_phi = GetPhiCandidate(user_id);
BasicBlock* bb = pass_->context()->get_instr_block(user_id);
if (user_phi) {
// If the user is a Phi candidate, replace all arguments that refer to
// |phi_to_remove.result_id()| with |repl_id|.
@ -110,6 +111,10 @@ void SSARewriter::ReplacePhiUsersWith(const PhiCandidate& phi_to_remove,
arg = repl_id;
}
}
} else if (bb->id() == user_id) {
// The phi candidate is the definition of the variable at basic block
// |bb|. We must change this to the replacement.
WriteVariable(phi_to_remove.var_id(), bb, repl_id);
} else {
// For regular loads, traverse the |load_replacement_| table looking for
// instances of |phi_to_remove|.
@ -259,6 +264,8 @@ uint32_t SSARewriter::GetReachingDef(uint32_t var_id, BasicBlock* bb) {
// require a Phi instruction. This will act as |var_id|'s current
// definition to break potential cycles.
PhiCandidate& phi_candidate = CreatePhiCandidate(var_id, bb);
// Set the value for |bb| to avoid an infinite recursion.
WriteVariable(var_id, bb, phi_candidate.result_id());
val_id = AddPhiOperands(&phi_candidate);
}

View File

@ -188,6 +188,9 @@ class SSARewriter {
// value |val_id|.
void WriteVariable(uint32_t var_id, BasicBlock* bb, uint32_t val_id) {
defs_at_block_[bb][var_id] = val_id;
if (auto* pc = GetPhiCandidate(val_id)) {
pc->AddUser(bb->id());
}
}
// Processes the store operation |inst| in basic block |bb|. This extracts

View File

@ -1930,6 +1930,63 @@ TEST_F(LocalSSAElimTest, VariablePointerTest2) {
SinglePassRunAndMatch<LocalMultiStoreElimPass>(text, false);
}
TEST_F(LocalSSAElimTest, ChainedTrivialPhis) {
// Check that the copy object get the undef value implicitly assigned in the
// entry block.
const std::string text = R"(
; CHECK: [[undef:%\w+]] = OpUndef %v4float
; CHECK: OpCopyObject %v4float [[undef]]
OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %2 "main"
OpExecutionMode %2 LocalSize 1 18 6
OpSource ESSL 310
%void = OpTypeVoid
%4 = OpTypeFunction %void
%bool = OpTypeBool
%float = OpTypeFloat 32
%v4float = OpTypeVector %float 4
%_ptr_Function_v4float = OpTypePointer Function %v4float
%2 = OpFunction %void None %4
%9 = OpLabel
%10 = OpVariable %_ptr_Function_v4float Function
OpBranch %11
%11 = OpLabel
OpLoopMerge %12 %13 None
OpBranch %14
%14 = OpLabel
%15 = OpUndef %bool
OpBranchConditional %15 %16 %12
%16 = OpLabel
%17 = OpUndef %bool
OpSelectionMerge %18 None
OpBranchConditional %17 %19 %18
%19 = OpLabel
%20 = OpUndef %bool
OpLoopMerge %21 %22 None
OpBranchConditional %20 %23 %21
%23 = OpLabel
%24 = OpLoad %v4float %10
%25 = OpCopyObject %v4float %24
%26 = OpUndef %bool
OpBranch %22
%22 = OpLabel
OpBranch %19
%21 = OpLabel
OpBranch %12
%18 = OpLabel
OpBranch %13
%13 = OpLabel
OpBranch %11
%12 = OpLabel
%27 = OpLoad %v4float %10
OpReturn
OpFunctionEnd
)";
SinglePassRunAndMatch<SSARewritePass>(text, false);
}
// TODO(greg-lunarg): Add tests to verify handling of these cases:
//
// No optimization in the presence of