diff --git a/source/opt/ccp_pass.cpp b/source/opt/ccp_pass.cpp index b32bed17..dc8aecd9 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 ba54dc7c..e96e731a 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 b6c4c181..6c8b854f 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 daec5bec..0fe4d6a1 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 195a304c..69469e03 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 1306c24b..24ffe348 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