AuroraMiddleware/SPIRV-Tools
1
0
Fork 0
mirror of https://github.com/KhronosGroup/SPIRV-Tools synced 2024-12-26 09:41:03 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix constant propagation of induction variables.

Browse Source 
This fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/1143.
When an instruction transitions from constant to bottom (varying) in the
lattice, we were telling the propagator that the instruction was
varying, but never updating the actual value in the values table.

This led to incorrect value substitutions at the end of propagation.

The patch also re-enables CCP in -O and -Os.
This commit is contained in:
Diego Novillo 2018-01-08 11:56:57 -05:00
parent a82a0ea886
commit e5560d64de
6 changed files with 140 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

57
source/opt/ccp_pass.cpp
View File

@ -27,6 +27,25 @@
namespace spvtools {
namespace opt {
namespace {
// This SSA id is never defined nor referenced in the IR. It is a special ID
// which represents varying values. When an ID is found to have a varying
// value, its entry in the |values_| table maps to kVaryingSSAId.
const uint32_t kVaryingSSAId = std::numeric_limits<uint32_t>::max();
} // namespace
bool CCPPass::IsVaryingValue(uint32_t id) const { return id == kVaryingSSAId; }
SSAPropagator::PropStatus CCPPass::MarkInstructionVarying(
ir::Instruction* instr) {
assert(instr->result_id() != 0 &&
"Instructions with no result cannot be marked varying.");
values_[instr->result_id()] = kVaryingSSAId;
return SSAPropagator::kVarying;
}
SSAPropagator::PropStatus CCPPass::VisitPhi(ir::Instruction* phi) {
uint32_t meet_val_id = 0;
@ -52,9 +71,10 @@ SSAPropagator::PropStatus CCPPass::VisitPhi(ir::Instruction* phi) {
// looking.
continue;
} else {
// We found another constant value, but it is different from the
// previous computed meet value. This Phi will never be constant.
return SSAPropagator::kVarying;
// We either found a varying value, or another constant value different
// from the previous computed meet value. This Phi will never be
// constant.
return MarkInstructionVarying(phi);
}
} else {
// If any argument is not a constant, the Phi produces nothing
@ -85,25 +105,34 @@ SSAPropagator::PropStatus CCPPass::VisitAssignment(ir::Instruction* instr) {
uint32_t rhs_id = instr->GetSingleWordInOperand(0);
auto it = values_.find(rhs_id);
if (it != values_.end()) {
values_[instr->result_id()] = it->second;
return SSAPropagator::kInteresting;
if (IsVaryingValue(it->second)) {
return MarkInstructionVarying(instr);
} else {
values_[instr->result_id()] = it->second;
return SSAPropagator::kInteresting;
}
}
return SSAPropagator::kNotInteresting;
}
// Instructions with a RHS that cannot produce a constant are always varying.
if (!instr->IsFoldable()) {
return SSAPropagator::kVarying;
return MarkInstructionVarying(instr);
}
// Otherwise, see if the RHS of the assignment folds into a constant value.
std::vector<uint32_t> cst_val_ids;
bool missing_constants = false;
instr->ForEachInId([this, &cst_val_ids, &missing_constants](uint32_t* op_id) {
bool varying_values = false;
instr->ForEachInId([this, &cst_val_ids, &missing_constants,
&varying_values](uint32_t* op_id) {
auto it = values_.find(*op_id);
if (it == values_.end()) {
missing_constants = true;
return;
} else if (IsVaryingValue(it->second)) {
varying_values = true;
return;
}
cst_val_ids.push_back(it->second);
});
@ -115,6 +144,12 @@ SSAPropagator::PropStatus CCPPass::VisitAssignment(ir::Instruction* instr) {
return SSAPropagator::kNotInteresting;
}
// If we found at least one varying value, the instruction will never fold
// into anything interesting. Mark it varying.
if (varying_values) {
return MarkInstructionVarying(instr);
}
auto constants = const_mgr_->GetConstantsFromIds(cst_val_ids);
assert(constants.size() != 0 && "Found undeclared constants");
@ -125,7 +160,7 @@ SSAPropagator::PropStatus CCPPass::VisitAssignment(ir::Instruction* instr) {
[](const analysis::Constant* cst) {
return IsFoldableConstant(cst);
})) {
return SSAPropagator::kVarying;
return MarkInstructionVarying(instr);
}
// Otherwise, fold the instruction with all the operands to produce a new
@ -154,7 +189,7 @@ SSAPropagator::PropStatus CCPPass::VisitBranch(ir::Instruction* instr,
// according to the selector's boolean value.
uint32_t pred_id = instr->GetSingleWordOperand(0);
auto it = values_.find(pred_id);
if (it == values_.end()) {
if (it == values_.end() || IsVaryingValue(it->second)) {
// The predicate has an unknown value, either branch could be taken.
return SSAPropagator::kVarying;
}
@ -179,7 +214,7 @@ SSAPropagator::PropStatus CCPPass::VisitBranch(ir::Instruction* instr,
}
uint32_t select_id = instr->GetSingleWordOperand(0);
auto it = values_.find(select_id);
if (it == values_.end()) {
if (it == values_.end() || IsVaryingValue(it->second)) {
// The selector has an unknown value, any of the branches could be taken.
return SSAPropagator::kVarying;
}
@ -225,7 +260,7 @@ bool CCPPass::ReplaceValues() {
for (const auto& it : values_) {
uint32_t id = it.first;
uint32_t cst_id = it.second;
if (id != cst_id) {
if (!IsVaryingValue(cst_id) && id != cst_id) {
retval |= context()->ReplaceAllUsesWith(id, cst_id);
}
}

13
source/opt/ccp_pass.h
View File

@ -67,6 +67,14 @@ class CCPPass : public MemPass {
// otherwise.
bool ReplaceValues();
// Marks |instr| as varying by registering a varying value for its result
// into the |values_| table. Returns SSAPropagator::kVarying.
SSAPropagator::PropStatus MarkInstructionVarying(ir::Instruction* instr);
// Returns true if |id| is the special SSA id that corresponds to a varying
// value.
bool IsVaryingValue(uint32_t id) const;
// Constant manager for the parent IR context. Used to record new constants
// generated during propagation.
analysis::ConstantManager* const_mgr_;
@ -75,6 +83,11 @@ class CCPPass : public MemPass {
// represents the compile-time constant value for |id| as declared by
// |const_decl_id|. Each |const_decl_id| in this table is an OpConstant
// declaration for the current module.
//
// Additionally, this table keeps track of SSA IDs with varying values. If an
// SSA ID is found to have a varying value, it will have an entry in this
// table that maps to the special SSA id kVaryingSSAId. These values are
// never replaced in the IR, they are used by CCP during propagation.
std::unordered_map<uint32_t, uint32_t> values_;
// Propagator engine used.

8
source/opt/optimizer.cpp
View File

@ -125,9 +125,7 @@ Optimizer& Optimizer::RegisterPerformancePasses() {
.RegisterPass(CreateLocalSingleStoreElimPass())
.RegisterPass(CreateInsertExtractElimPass())
.RegisterPass(CreateLocalMultiStoreElimPass())
// TODO(dneto): Disable CCP until it optimizes loops correctly
// https://github.com/KhronosGroup/SPIRV-Tools/issues/1143
//.RegisterPass(CreateCCPPass())
.RegisterPass(CreateCCPPass())
.RegisterPass(CreateAggressiveDCEPass())
.RegisterPass(CreateDeadBranchElimPass())
.RegisterPass(CreateBlockMergePass())
@ -149,9 +147,7 @@ Optimizer& Optimizer::RegisterSizePasses() {
.RegisterPass(CreateLocalSingleStoreElimPass())
.RegisterPass(CreateInsertExtractElimPass())
.RegisterPass(CreateLocalMultiStoreElimPass())
// TODO(dneto): Disable CCP until it optimizes loops correctly
// https://github.com/KhronosGroup/SPIRV-Tools/issues/1143
//.RegisterPass(CreateCCPPass())
.RegisterPass(CreateCCPPass())
.RegisterPass(CreateAggressiveDCEPass())
.RegisterPass(CreateDeadBranchElimPass())
.RegisterPass(CreateBlockMergePass())

18
source/opt/propagator.cpp
View File

@ -37,18 +37,24 @@ void SSAPropagator::AddControlEdge(const Edge& edge) {
}
void SSAPropagator::AddSSAEdges(ir::Instruction* instr) {
if (instr->result_id() == 0 || instr->opcode() == SpvOpPhi) {
// Ignore instructions that produce no result and Phi instructions. The SSA
// edges out of Phi instructions are never added. Phi instructions can
// produce cycles in the def-use web and they are always simulated when a
// block is visited.
// Ignore instructions that produce no result.
if (instr->result_id() == 0) {
return;
}
get_def_use_mgr()->ForEachUser(
instr->result_id(), [this](ir::Instruction* use_instr) {
// If |use_instr| is a Phi, ignore this edge. Phi instructions can form
// cycles in the def-use web, which would get the propagator into an
// infinite loop. Phi instructions are always simulated when a block is
// visited, so there is no need to traverse the SSA edges into them.
if (use_instr->opcode() == SpvOpPhi) {
return;
}
// If the basic block for |use_instr| has not been simulated yet, do
// nothing.
// nothing. The instruction |use_instr| will be simulated next time the
// block is scheduled.
if (!BlockHasBeenSimulated(ctx_->get_instr_block(use_instr))) {
return;
}

10
source/opt/propagator.h
View File

@ -254,11 +254,11 @@ class SSAPropagator {
// Adds all the instructions that use the result of |instr| to the SSA edges
// work list. If |instr| produces no result id, this does nothing. This also
// does nothing if |instr| is a Phi instruction. Phi instructions are treated
// specially because (a) they can be in def-use cycles with other
// instructions, and (b) they are always executed when a basic block is
// simulated (see the description of the Sparse Conditional Constant algorithm
// in the original paper).
// does nothing if the instruction at the end of the def-use is a Phi
// instruction. Phi instructions are treated specially because (a) they can
// be in def-use cycles with other Phi instructions, and (b) they are always
// executed when a basic block is simulated (see the description of the Sparse
// Conditional Constant algorithm in the original paper).
void AddSSAEdges(ir::Instruction* instr);
// IR context to use.

65
test/opt/ccp_test.cpp
View File

@ -369,12 +369,16 @@ TEST_F(CCPTest, NoLoadStorePropagation) {
OpStore %x %int_23
; int_23 should not propagate into this load.
; CHECK: OpLoad %int %x
; CHECK: [[load_id:%\d+]] = OpLoad %int %x
%12 = OpLoad %int %x
; Nor into this copy operation.
; CHECK: [[copy_id:%\d+]] = OpCopyObject %int [[load_id]]
%13 = OpCopyObject %int %12
; Likewise here.
; CHECK: OpStore %outparm {{%\d+}}
OpStore %outparm %12
; CHECK: OpStore %outparm [[copy_id]]
OpStore %outparm %13
OpReturn
OpFunctionEnd
)";
@ -464,6 +468,61 @@ TEST_F(CCPTest, SSAWebCycles) {
SinglePassRunAndMatch<opt::CCPPass>(spv_asm, true);
}
TEST_F(CCPTest, LoopInductionVariables) {
// Test reduced from https://github.com/KhronosGroup/SPIRV-Tools/issues/1143
// We are failing to properly consider the induction variable for this loop
// as Varying.
const std::string spv_asm = R"(
OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %main "main"
OpExecutionMode %main OriginUpperLeft
OpSource GLSL 430
OpName %main "main"
%void = OpTypeVoid
%5 = OpTypeFunction %void
%int = OpTypeInt 32 1
%_ptr_Function_int = OpTypePointer Function %int
%int_0 = OpConstant %int 0
%int_10 = OpConstant %int 10
%bool = OpTypeBool
%int_1 = OpConstant %int 1
%main = OpFunction %void None %5
%12 = OpLabel
OpBranch %13
%13 = OpLabel
; This Phi should not have all constant arguments:
; CHECK: [[phi_id:%\d+]] = OpPhi %int %int_0 {{%\d+}} {{%\d+}} {{%\d+}}
%22 = OpPhi %int %int_0 %12 %21 %15
OpLoopMerge %14 %15 None
OpBranch %16
%16 = OpLabel
; The Phi should never be considered to have the value %int_0.
; CHECK: [[branch_selector:%\d+]] = OpSLessThan %bool [[phi_id]] %int_10
%18 = OpSLessThan %bool %22 %int_10
; This conditional was wrongly converted into an always-true jump due to the
; bad meet evaluation of %22.
; CHECK: OpBranchConditional [[branch_selector]] {{%\d+}} {{%\d+}}
OpBranchConditional %18 %19 %14
%19 = OpLabel
OpBranch %15
%15 = OpLabel
; CHECK: OpIAdd %int [[phi_id]] %int_1
%21 = OpIAdd %int %22 %int_1
OpBranch %13
%14 = OpLabel
OpReturn
OpFunctionEnd
)";
SinglePassRunAndMatch<opt::CCPPass>(spv_asm, true);
}
#endif
} // namespace