From e5560d64de40bcc52b38ee44fd8a0bbc42f64399 Mon Sep 17 00:00:00 2001 From: Diego Novillo Date: Mon, 8 Jan 2018 11:56:57 -0500 Subject: [PATCH] Fix constant propagation of induction variables. 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. --- source/opt/ccp_pass.cpp | 57 +++++++++++++++++++++++++++------- source/opt/ccp_pass.h | 13 ++++++++ source/opt/optimizer.cpp | 8 ++--- source/opt/propagator.cpp | 18 +++++++---- source/opt/propagator.h | 10 +++--- test/opt/ccp_test.cpp | 65 +++++++++++++++++++++++++++++++++++++-- 6 files changed, 140 insertions(+), 31 deletions(-) diff --git a/source/opt/ccp_pass.cpp b/source/opt/ccp_pass.cpp index b32bed17f..dc8aecd99 100644 --- a/source/opt/ccp_pass.cpp +++ b/source/opt/ccp_pass.cpp @@ -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::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 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); } } diff --git a/source/opt/ccp_pass.h b/source/opt/ccp_pass.h index ba54dc7c0..e96e731a7 100644 --- a/source/opt/ccp_pass.h +++ b/source/opt/ccp_pass.h @@ -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 values_; // Propagator engine used. diff --git a/source/opt/optimizer.cpp b/source/opt/optimizer.cpp index b6c4c1811..6c8b854f2 100644 --- a/source/opt/optimizer.cpp +++ b/source/opt/optimizer.cpp @@ -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()) diff --git a/source/opt/propagator.cpp b/source/opt/propagator.cpp index daec5bec1..0fe4d6a16 100644 --- a/source/opt/propagator.cpp +++ b/source/opt/propagator.cpp @@ -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; } diff --git a/source/opt/propagator.h b/source/opt/propagator.h index 195a304c8..69469e03e 100644 --- a/source/opt/propagator.h +++ b/source/opt/propagator.h @@ -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. diff --git a/test/opt/ccp_test.cpp b/test/opt/ccp_test.cpp index 1306c24b2..24ffe3484 100644 --- a/test/opt/ccp_test.cpp +++ b/test/opt/ccp_test.cpp @@ -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(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(spv_asm, true); +} + #endif } // namespace