Fix https://github.com/KhronosGroup/SPIRV-Tools/issues/4462 (#4651)
This prevents CCP from making constant -> constant transitions when
evaluating instruction values. In this case, 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.
diff --git a/source/opt/ccp_pass.cpp b/source/opt/ccp_pass.cpp
index 8b896d5..5099b47 100644
--- a/source/opt/ccp_pass.cpp
+++ b/source/opt/ccp_pass.cpp
@@ -102,6 +102,34 @@
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 @@
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 @@
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 fb20c78..77ea9f8 100644
--- a/source/opt/ccp_pass.h
+++ b/source/opt/ccp_pass.h
@@ -92,6 +92,22 @@
// 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 <id, const_decl_id> 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 212e051..ae7043b 100644
--- a/test/opt/ccp_test.cpp
+++ b/test/opt/ccp_test.cpp
@@ -1234,6 +1234,86 @@
SinglePassRunAndCheck<CCPPass>(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<CCPPass>(text, true);
+ EXPECT_EQ(std::get<1>(result), Pass::Status::SuccessWithChange);
+}
+
} // namespace
} // namespace opt
} // namespace spvtools
