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.

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.

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 <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

test/opt/ccp_test.cpp

@@ -1234,6 +1234,86 @@ OpFunctionEnd
 
   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