diff --git a/source/opt/ccp_pass.cpp b/source/opt/ccp_pass.cpp index 8b896d504..5099b477a 100644 --- a/source/opt/ccp_pass.cpp +++ b/source/opt/ccp_pass.cpp @@ -102,6 +102,34 @@ SSAPropagator::PropStatus CCPPass::VisitPhi(Instruction* phi) { return SSAPropagator::kInteresting; } +uint32_t CCPPass::ComputeLatticeMeet(Instruction* instr, uint32_t val2) { + // Given two values val1 and val2, the meet operation in the constant + // lattice uses the following rules: + // + // meet(val1, UNDEFINED) = val1 + // meet(val1, VARYING) = VARYING + // meet(val1, val2) = val1 if val1 == val2 + // meet(val1, val2) = VARYING if val1 != val2 + // + // When two different values meet, the result is always varying because CCP + // does not allow lateral transitions in the lattice. This prevents + // infinite cycles during propagation. + auto val1_it = values_.find(instr->result_id()); + if (val1_it == values_.end()) { + return val2; + } + + uint32_t val1 = val1_it->second; + if (IsVaryingValue(val1)) { + return val1; + } else if (IsVaryingValue(val2)) { + return val2; + } else if (val1 != val2) { + return kVaryingSSAId; + } + return val2; +} + SSAPropagator::PropStatus CCPPass::VisitAssignment(Instruction* instr) { assert(instr->result_id() != 0 && "Expecting an instruction that produces a result"); @@ -115,8 +143,10 @@ SSAPropagator::PropStatus CCPPass::VisitAssignment(Instruction* instr) { if (IsVaryingValue(it->second)) { return MarkInstructionVarying(instr); } else { - values_[instr->result_id()] = it->second; - return SSAPropagator::kInteresting; + uint32_t new_val = ComputeLatticeMeet(instr, it->second); + values_[instr->result_id()] = new_val; + return IsVaryingValue(new_val) ? SSAPropagator::kVarying + : SSAPropagator::kInteresting; } } return SSAPropagator::kNotInteresting; @@ -142,9 +172,12 @@ SSAPropagator::PropStatus CCPPass::VisitAssignment(Instruction* instr) { if (folded_inst != nullptr) { // We do not want to change the body of the function by adding new // instructions. When folding we can only generate new constants. - assert(folded_inst->IsConstant() && "CCP is only interested in constant."); - values_[instr->result_id()] = folded_inst->result_id(); - return SSAPropagator::kInteresting; + assert(folded_inst->IsConstant() && + "CCP is only interested in constant values."); + uint32_t new_val = ComputeLatticeMeet(instr, folded_inst->result_id()); + values_[instr->result_id()] = new_val; + return IsVaryingValue(new_val) ? SSAPropagator::kVarying + : SSAPropagator::kInteresting; } // Conservatively mark this instruction as varying if any input id is varying. diff --git a/source/opt/ccp_pass.h b/source/opt/ccp_pass.h index fb20c7808..77ea9f80d 100644 --- a/source/opt/ccp_pass.h +++ b/source/opt/ccp_pass.h @@ -92,6 +92,22 @@ class CCPPass : public MemPass { // generated during propagation. analysis::ConstantManager* const_mgr_; + // Returns a new value for |instr| by computing the meet operation between + // its existing value and |val2|. + // + // Given two values val1 and val2, the meet operation in the constant + // lattice uses the following rules: + // + // meet(val1, UNDEFINED) = val1 + // meet(val1, VARYING) = VARYING + // meet(val1, val2) = val1 if val1 == val2 + // meet(val1, val2) = VARYING if val1 != val2 + // + // When two different values meet, the result is always varying because CCP + // does not allow lateral transitions in the lattice. This prevents + // infinite cycles during propagation. + uint32_t ComputeLatticeMeet(Instruction* instr, uint32_t val2); + // Constant value table. Each entry in this map // represents the compile-time constant value for |id| as declared by // |const_decl_id|. Each |const_decl_id| in this table is an OpConstant diff --git a/test/opt/ccp_test.cpp b/test/opt/ccp_test.cpp index 212e05140..ae7043b9a 100644 --- a/test/opt/ccp_test.cpp +++ b/test/opt/ccp_test.cpp @@ -1234,6 +1234,86 @@ OpFunctionEnd SinglePassRunAndCheck(text, text, false); } + +// Test from https://github.com/KhronosGroup/SPIRV-Tools/issues/4462. +// The test was causing a lateral movement in the constant lattice, which was +// not being detected as varying by CCP. In this test, FClamp is evaluated +// twice. On the first evaluation, if computes FClamp(0.5, 0.5, -1) which +// returns -1. On the second evaluation, it computes FClamp(0.5, 0.5, VARYING) +// which returns 0.5. +// +// Both fold() computations are correct given the semantics of FClamp() but +// this causes a lateral transition in the constant lattice which was not being +// considered VARYING by CCP. +TEST_F(CCPTest, LateralLatticeTransition) { + const std::string text = R"(OpCapability Shader + %1 = OpExtInstImport "GLSL.std.450" + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %main "main" %gl_FragCoord %outColor + OpExecutionMode %main OriginUpperLeft + OpSource ESSL 310 + OpName %main "main" + OpName %gl_FragCoord "gl_FragCoord" + OpName %outColor "outColor" + OpDecorate %gl_FragCoord BuiltIn FragCoord + OpDecorate %outColor Location 0 + %void = OpTypeVoid + %6 = OpTypeFunction %void + %float = OpTypeFloat 32 + %float_0_5 = OpConstant %float 0.5 + %v4float = OpTypeVector %float 4 +%_ptr_Input_v4float = OpTypePointer Input %v4float +%gl_FragCoord = OpVariable %_ptr_Input_v4float Input + %uint = OpTypeInt 32 0 + %uint_0 = OpConstant %uint 0 +%_ptr_Input_float = OpTypePointer Input %float + %float_0 = OpConstant %float 0 + %bool = OpTypeBool + %float_n1 = OpConstant %float -1 + %float_1 = OpConstant %float 1 +%_ptr_Output_v4float = OpTypePointer Output %v4float + %outColor = OpVariable %_ptr_Output_v4float Output + +; This constant is created during the first evaluation of the CompositeConstruct +; CHECK: [[new_constant:%\d+]] = OpConstantComposite %v4float %float_n1 %float_0_5 %float_0 %float_1 + + %main = OpFunction %void None %6 + %19 = OpLabel + %20 = OpAccessChain %_ptr_Input_float %gl_FragCoord %uint_0 + %21 = OpLoad %float %20 + %22 = OpFOrdLessThan %bool %21 %float_0 + OpSelectionMerge %23 None + OpBranchConditional %22 %24 %25 + %24 = OpLabel + OpBranch %23 + %25 = OpLabel + OpBranch %26 + %26 = OpLabel + OpBranch %23 + %23 = OpLabel + %27 = OpPhi %float %float_n1 %24 %float_0_5 %26 + %28 = OpExtInst %float %1 FClamp %float_0_5 %float_0_5 %27 + + ; On first evaluation, the result from FClamp will return 0.5. + ; But on second evaluation, FClamp should return VARYING. Check + ; that CCP is not keeping the first result. + ; CHECK-NOT: %29 = OpCompositeConstruct %v4float %float_0_5 %float_0_5 %float_0 %float_1 + %29 = OpCompositeConstruct %v4float %28 %float_0_5 %float_0 %float_1 + + ; CHECK-NOT: OpCopyObject %v4float [[new_constant]] + %42 = OpCopyObject %v4float %29 + + ; CHECK-NOT: OpStore %outColor [[new_constant]] + OpStore %outColor %42 + + OpReturn + OpFunctionEnd +)"; + + auto result = SinglePassRunAndMatch(text, true); + EXPECT_EQ(std::get<1>(result), Pass::Status::SuccessWithChange); +} + } // namespace } // namespace opt } // namespace spvtools