Repository: KhronosGroup/SPIRV-Tools (language: python)

Issue:
[bug] Incorrect optimization on constant propagation and eliminate-local-multi-store

The spirv-opt tool incorrectly optimizes the modulo operation with constant operands when enabling the `--ccp` and `--eliminate-local-multi-store` passes.

## Description of the issue

The following shader is a minimal example that reproduces the issue:

```glsl
#version 310 es

precision highp float;
precision highp int;

layout(location = 0) out vec4 fragColor;

void main() {
   int t = 300000000;
   uint x = uint(t % 2000000001);   // The expected value of x is 300000000
   float ret = float(x % 2u);     // The expected value of ret is 0.0, yet the actual result is 1.0
   fragColor = vec4(ret, ret, ret, 1.0);
}
```

The expected value of `ret` is 0.0, because the value of `x` is 300000000, which is divisible by 2. So the expected color of the fragment is (0.0, 0.0, 0.0, 1.0), i.e., a black fragment.

However, when optimizing the compiled spv file with the `--ccp` and `--eliminate-local-multi-store` passes, the fragment color is incorrectly white, i.e., (1.0, 1.0, 1.0, 1.0).

Note that the constant values and intermediate computations are within valid ranges for the respective types, so it should not be due to the undefined behavior.

## Further investigation of the binary

I further use the [RenderDoc](https://renderdoc.org/) tool to debug the shader. The following is the disassembly of the shader:

```c
void main() {
  int* t;
  uint* x;
  float* ret;
  *t = 300000000;
  int _46 = SMod(300000000, 2000000001);
  uint _47 = Bitcast(-1994967295);
  *x = _47;
  uint _60 = UMod(_47, 2);
  float _61 = ConvertUToF(_60);
  *ret = _61;
  float4 _75 = CompositeConstruct({_61, _61, _61, 1.0000});
  *fragColor = _75;
  return;
}
```

We can see that the value of variable `x` is assigned from that of variable `_47`, which is `Bitcast(-1994967295)`, or 2300000001 after being bit-cast to unsigned integer.
2300000001 is equal to 2000000001 + 300000000, which is equivalent to adding two operands of the modulo operation.

Such a value of `x` is different from the expected operation of 300000000 % 2000000001 = 300000000 as in the semantics of the glsl shader.

Later, the value of `x` is used in the modulo operation with 2, which gives 2300000001 % 2 = 1, causing the value of `ret` to be 1.0 instead of the expected 0.0.

## Steps to reproduce

Minimal commands to compile & optimize the glsl shader to spv:

```bash
glslang -gVS -o frag.non-opt.spv --target-env vulkan1.3 [file-of-glsl-shader-above].frag
spirv-opt frag.non-opt.spv -o frag.spv --eliminate-local-multi-store --ccp
```

I provide a miminal package that reproduces the issue. The package contains an application that draws a triangle with the incorrectly optimized spirv shader. It also describes the environment, hardware, and steps to reproduce the issue. The capture file of RenderDoc is also included. You can download the package from [here](https://www.dropbox.com/scl/fi/tyllm34lrk9z98eevc8ud/ccp-bug-report.zip?rlkey=xjgb6ymodhidw6emdlo5y3h6p&dl=0)



Discussion / hints:
Look like I fixed the surface issue with #5561. However, I do not understand how we were getting the earlier result in the first place. I did not think that the OpSMod would have been folded, so it seems like there might be another bug that is now hidden. I'll leave this open while I try to determine what was happening before my change.
The code that originally folded the OpSMod, would do an `OSRem` and then try to "fix" the value to account for the difference in definition:

https://github.com/KhronosGroup/SPIRV-Tools/blob/784b064f90a597d2478064161438f4986485b5d4/source/opt/fold.cpp#L109-L111

The problem is that the third line overflows in this case, and give an incorrect value. Since we have a new implementation for OpSMod folding, which does not have this problem. I will simply delete that code.

Fix patch:
diff --git a/source/opt/const_folding_rules.cpp b/source/opt/const_folding_rules.cpp
index 79f34acd3c..17900af245 100644
--- a/source/opt/const_folding_rules.cpp
+++ b/source/opt/const_folding_rules.cpp
@@ -1715,8 +1715,10 @@ BinaryScalarFoldingRule FoldBinaryIntegerOperation(uint64_t (*op)(uint64_t,
         assert(result_type != nullptr && a != nullptr && b != nullptr);
         const analysis::Integer* integer_type = result_type->AsInteger();
         assert(integer_type != nullptr);
-        assert(integer_type == a->type()->AsInteger());
-        assert(integer_type == b->type()->AsInteger());
+        assert(a->type()->kind() == analysis::Type::kInteger);
+        assert(b->type()->kind() == analysis::Type::kInteger);
+        assert(integer_type->width() == a->type()->AsInteger()->width());
+        assert(integer_type->width() == b->type()->AsInteger()->width());
 
         // In SPIR-V, all operations support unsigned types, but the way they
         // are interpreted depends on the opcode. This is why we use the
diff --git a/source/opt/fold.cpp b/source/opt/fold.cpp
index c2a97b6e6e..942da6835f 100644
--- a/source/opt/fold.cpp
+++ b/source/opt/fold.cpp
@@ -70,58 +70,6 @@ uint32_t InstructionFolder::UnaryOperate(spv::Op opcode,
 uint32_t InstructionFolder::BinaryOperate(spv::Op opcode, uint32_t a,
                                           uint32_t b) const {
   switch (opcode) {
-    // Arthimetics
-    case spv::Op::OpIAdd:
-      return a + b;
-    case spv::Op::OpISub:
-      return a - b;
-    case spv::Op::OpIMul:
-      return a * b;
-    case spv::Op::OpUDiv:
-      if (b != 0) {
-        return a / b;
-      } else {
-        // Dividing by 0 is undefined, so we will just pick 0.
-        return 0;
-      }
-    case spv::Op::OpSDiv:
-      if (b != 0u) {
-        return (static_cast<int32_t>(a)) / (static_cast<int32_t>(b));
-      } else {
-        // Dividing by 0 is undefined, so we will just pick 0.
-        return 0;
-      }
-    case spv::Op::OpSRem: {
-      // The sign of non-zero result comes from the first operand: a. This is
-      // guaranteed by C++11 rules for integer division operator. The division
-      // result is rounded toward zero, so the result of '%' has the sign of
-      // the first operand.
-      if (b != 0u) {
-        return static_cast<int32_t>(a) % static_cast<int32_t>(b);
-      } else {
-        // Remainder when dividing with 0 is undefined, so we will just pick 0.
-        return 0;
-      }
-    }
-    case spv::Op::OpSMod: {
-      // The sign of non-zero result comes from the second operand: b
-      if (b != 0u) {
-        int32_t rem = BinaryOperate(spv::Op::OpSRem, a, b);
-        int32_t b_prim = static_cast<int32_t>(b);
-        return (rem + b_prim) % b_prim;
-      } else {
-        // Mod with 0 is undefined, so we will just pick 0.
-        return 0;
-      }
-    }
-    case spv::Op::OpUMod:
-      if (b != 0u) {
-        return (a % b);
-      } else {
-        // Mod with 0 is undefined, so we will just pick 0.
-        return 0;
-      }
-
     // Shifting
     case spv::Op::OpShiftRightLogical:
       if (b >= 32) {
diff --git a/source/opt/fold_spec_constant_op_and_composite_pass.cpp b/source/opt/fold_spec_constant_op_and_composite_pass.cpp
index 770e1fe50c..c568027d2a 100644
--- a/source/opt/fold_spec_constant_op_and_composite_pass.cpp
+++ b/source/opt/fold_spec_constant_op_and_composite_pass.cpp
@@ -115,20 +115,9 @@ bool FoldSpecConstantOpAndCompositePass::ProcessOpSpecConstantOp(
          "The first in-operand of OpSpecConstantOp instruction must be of "
          "SPV_OPERAND_TYPE_SPEC_CONSTANT_OP_NUMBER type");
 
-  switch (static_cast<spv::Op>(inst->GetSingleWordInOperand(0))) {
-    case spv::Op::OpCompositeExtract:
-    case spv::Op::OpVectorShuffle:
-    case spv::Op::OpCompositeInsert:
-    case spv::Op::OpQuantizeToF16:
-      folded_inst = FoldWithInstructionFolder(pos);
-      break;
-    default:
-      // TODO: This should use the instruction folder as well, but some folding
-      // rules are missing.
-
-      // Component-wise operations.
-      folded_inst = DoComponentWiseOperation(pos);
-      break;
+  folded_inst = FoldWithInstructionFolder(pos);
+  if (!folded_inst) {
+    folded_inst = DoComponentWiseOperation(pos);
   }
   if (!folded_inst) return false;
 


Test patch:
diff --git a/test/opt/fold_spec_const_op_composite_test.cpp b/test/opt/fold_spec_const_op_composite_test.cpp
index 70ba362056..335e0f5162 100644
--- a/test/opt/fold_spec_const_op_composite_test.cpp
+++ b/test/opt/fold_spec_const_op_composite_test.cpp
@@ -1058,14 +1058,8 @@ INSTANTIATE_TEST_SUITE_P(
               },
               // expected
               {
-                "%true = OpConstantTrue %bool",
-                "%true_0 = OpConstantTrue %bool",
                 "%spec_bool_t_vec = OpConstantComposite %v2bool %bool_true %bool_true",
-                "%false = OpConstantFalse %bool",
-                "%false_0 = OpConstantFalse %bool",
                 "%spec_bool_f_vec = OpConstantComposite %v2bool %bool_false %bool_false",
-                "%false_1 = OpConstantFalse %bool",
-                "%false_2 = OpConstantFalse %bool",
                 "%spec_bool_from_null = OpConstantComposite %v2bool %bool_false %bool_false",
               },
             },
@@ -1080,14 +1074,8 @@ INSTANTIATE_TEST_SUITE_P(
               },
               // expected
               {
-                "%true = OpConstantTrue %bool",
-                "%true_0 = OpConstantTrue %bool",
                 "%spec_bool_t_vec = OpConstantComposite %v2bool %bool_true %bool_true",
-                "%false = OpConstantFalse %bool",
-                "%false_0 = OpConstantFalse %bool",
                 "%spec_bool_f_vec = OpConstantComposite %v2bool %bool_false %bool_false",
-                "%false_1 = OpConstantFalse %bool",
-                "%false_2 = OpConstantFalse %bool",
                 "%spec_bool_from_null = OpConstantComposite %v2bool %bool_false %bool_false",
               },
             },
@@ -1102,14 +1090,8 @@ INSTANTIATE_TEST_SUITE_P(
               },
               // expected
               {
-                "%int_1 = OpConstant %int 1",
-                "%int_1_0 = OpConstant %int 1",
                 "%spec_int_one_vec = OpConstantComposite %v2int %signed_one %signed_one",
-                "%int_0 = OpConstant %int 0",
-                "%int_0_0 = OpConstant %int 0",
                 "%spec_int_zero_vec = OpConstantComposite %v2int %signed_zero %signed_zero",
-                "%int_0_1 = OpConstant %int 0",
-                "%int_0_2 = OpConstant %int 0",
                 "%spec_int_from_null = OpConstantComposite %v2int %signed_zero %signed_zero",
               },
             },
@@ -1124,14 +1106,8 @@ INSTANTIATE_TEST_SUITE_P(
               },
               // expected
               {
-                "%int_1 = OpConstant %int 1",
-                "%int_1_0 = OpConstant %int 1",
                 "%spec_int_one_vec = OpConstantComposite %v2int %signed_one %signed_one",
-                "%int_0 = OpConstant %int 0",
-                "%int_0_0 = OpConstant %int 0",
                 "%spec_int_zero_vec = OpConstantComposite %v2int %signed_zero %signed_zero",
-                "%int_0_1 = OpConstant %int 0",
-                "%int_0_2 = OpConstant %int 0",
                 "%spec_int_from_null = OpConstantComposite %v2int %signed_zero %signed_zero",
               },
             },
@@ -1146,14 +1122,8 @@ INSTANTIATE_TEST_SUITE_P(
               },
               // expected
               {
-                "%uint_1 = OpConstant %uint 1",
-                "%uint_1_0 = OpConstant %uint 1",
                 "%spec_uint_one_vec = OpConstantComposite %v2uint %unsigned_one %unsigned_one",
-                "%uint_0 = OpConstant %uint 0",
-                "%uint_0_0 = OpConstant %uint 0",
                 "%spec_uint_zero_vec = OpConstantComposite %v2uint %unsigned_zero %unsigned_zero",
-                "%uint_0_1 = OpConstant %uint 0",
-                "%uint_0_2 = OpConstant %uint 0",
                 "%spec_uint_from_null = OpConstantComposite %v2uint %unsigned_zero %unsigned_zero",
               },
             },
@@ -1168,14 +1138,8 @@ INSTANTIATE_TEST_SUITE_P(
               },
               // expected
               {
-                "%uint_1 = OpConstant %uint 1",
-                "%uint_1_0 = OpConstant %uint 1",
                 "%spec_uint_one_vec = OpConstantComposite %v2uint %unsigned_one %unsigned_one",
-                "%uint_0 = OpConstant %uint 0",
-                "%uint_0_0 = OpConstant %uint 0",
                 "%spec_uint_zero_vec = OpConstantComposite %v2uint %unsigned_zero %unsigned_zero",
-                "%uint_0_1 = OpConstant %uint 0",
-                "%uint_0_2 = OpConstant %uint 0",
                 "%spec_uint_from_null = OpConstantComposite %v2uint %unsigned_zero %unsigned_zero",
               },
             },
@@ -1184,8 +1148,6 @@ INSTANTIATE_TEST_SUITE_P(
             {
               // original
               {
-                "%spec_uint_zero = OpSpecConstantOp %uint UConvert %bool_false",
-                "%spec_uint_one = OpSpecConstantOp %uint UConvert %bool_true",
                 "%spec_ulong_zero = OpSpecConstantOp %ulong UConvert %unsigned_zero",
                 "%spec_ulong_one = OpSpecConstantOp %ulong UConvert %unsigned_one",
                 "%spec_short_zero = OpSpecConstantOp %ushort UConvert %unsigned_zero",
@@ -1197,8 +1159,6 @@ INSTANTIATE_TEST_SUITE_P(
               },
               // expected
               {
-                "%spec_uint_zero = OpConstant %uint 0",
-                "%spec_uint_one = OpConstant %uint 1",
                 "%spec_ulong_zero = OpConstant %ulong 0",
                 "%spec_ulong_one = OpConstant %ulong 1",
                 "%spec_short_zero = OpConstant %ushort 0",
@@ -1236,24 +1196,13 @@ INSTANTIATE_TEST_SUITE_P(
             {
               // original
               {
-                "%spec_v2uint_zero = OpSpecConstantOp %v2uint UConvert %bool_false_vec",
-                "%spec_v2uint_one = OpSpecConstantOp %v2uint UConvert %bool_true_vec",
                 "%spec_v2ulong_zero = OpSpecConstantOp %v2ulong UConvert %unsigned_zero_vec",
                 "%spec_v2ulong_one = OpSpecConstantOp %v2ulong UConvert %unsigned_one_vec",
               },
               // expected
               {
-                "%uint_0 = OpConstant %uint 0",
-                "%uint_0_0 = OpConstant %uint 0",
-                "%spec_v2uint_zero = OpConstantComposite %v2uint %unsigned_zero %unsigned_zero",
-                "%uint_1 = OpConstant %uint 1",
-                "%uint_1_0 = OpConstant %uint 1",
-                "%spec_v2uint_one = OpConstantComposite %v2uint %unsigned_one %unsigned_one",
-                "%ulong_0 = OpConstant %ulong 0",
-                "%ulong_0_0 = OpConstant %ulong 0",
                 "%spec_v2ulong_zero = OpConstantComposite %v2ulong %ulong_zero %ulong_zero",
                 "%ulong_1 = OpConstant %ulong 1",
-                "%ulong_1_0 = OpConstant %ulong 1",
                 "%spec_v2ulong_one = OpConstantComposite %v2ulong %ulong_1 %ulong_1",
               },
             },
@@ -1268,14 +1217,10 @@ INSTANTIATE_TEST_SUITE_P(
               },
               // expected
               {
-                "%long_0 = OpConstant %long 0",
-                "%long_0_0 = OpConstant %long 0",
                 "%spec_v2long_zero = OpConstantComposite %v2long %long_zero %long_zero",
                 "%long_1 = OpConstant %long 1",
-                "%long_1_0 = OpConstant %long 1",
                 "%spec_v2long_one = OpConstantComposite %v2long %long_1 %long_1",
                 "%long_n1 = OpConstant %long -1",
-                "%long_n1_0 = OpConstant %long -1",
                 "%spec_v2long_minus_one = OpConstantComposite %v2long %long_n1 %long_n1",
               },
             },
@@ -1372,7 +1317,7 @@ INSTANTIATE_TEST_SUITE_P(
               {
                 "%int_minus_1 = OpConstant %int -1",
                 "%int_minus_2 = OpConstant %int -2",
-                "%int_neg_null = OpConstant %int 0",
+                "%int_neg_null = OpConstantNull %int",
                 "%int_max = OpConstant %int 2147483647",
                 "%int_neg_max = OpConstant %int -2147483647",
               },
@@ -1553,15 +1498,10 @@ INSTANTIATE_TEST_SUITE_P(
               },
               // expected
               {
-                "%int_n1 = OpConstant %int -1",
-                "%int_n1_0 = OpConstant %int -1",
                 "%v2int_minus_1 = OpConstantComposite %v2int %signed_minus_one %signed_minus_one",
                 "%int_n2 = OpConstant %int -2",
-                "%int_n2_0 = OpConstant %int -2",
                 "%v2int_minus_2 = OpConstantComposite %v2int %int_n2 %int_n2",
-                "%int_0 = OpConstant %int 0",
-                "%int_0_0 = OpConstant %int 0",
-                "%v2int_neg_null = OpConstantComposite %v2int %signed_zero %signed_zero",
+                "%v2int_neg_null = OpConstantComposite %v2int %signed_null %signed_null",
               },
             },
             // vector integer (including null vetors) add, sub, div, mul
@@ -1583,35 +1523,23 @@ INSTANTIATE_TEST_SUITE_P(
               // expected
               {
                 "%int_5 = OpConstant %int 5",
-                "%int_5_0 = OpConstant %int 5",
                 "%spec_v2int_iadd = OpConstantComposite %v2int %int_5 %int_5",
                 "%int_n4 = OpConstant %int -4",
-                "%int_n4_0 = OpConstant %int -4",
                 "%spec_v2int_isub = OpConstantComposite %v2int %int_n4 %int_n4",
                 "%int_n2 = OpConstant %int -2",
-                "%int_n2_0 = OpConstant %int -2",
                 "%spec_v2int_sdiv = OpConstantComposite %v2int %int_n2 %int_n2",
                 "%int_n6 = OpConstant %int -6",
-                "%int_n6_0 = OpConstant %int -6",
                 "%spec_v2int_imul = OpConstantComposite %v2int %int_n6 %int_n6",
-                "%int_n6_1 = OpConstant %int -6",
-                "%int_n6_2 = OpConstant %int -6",
                 "%spec_v2int_iadd_null = OpConstantComposite %v2int %int_n6 %int_n6",
 
                 "%uint_5 = OpConstant %uint 5",
-                "%uint_5_0 = OpConstant %uint 5",
                 "%spec_v2uint_iadd = OpConstantComposite %v2uint %uint_5 %uint_5",
                 "%uint_4294967292 = OpConstant %uint 4294967292",
-                "%uint_4294967292_0 = OpConstant %uint 4294967292",
                 "%spec_v2uint_isub = OpConstantComposite %v2uint %uint_4294967292 %uint_4294967292",
                 "%uint_1431655764 = OpConstant %uint 1431655764",
-                "%uint_1431655764_0 = OpConstant %uint 1431655764",
                 "%spec_v2uint_udiv = OpConstantComposite %v2uint %uint_1431655764 %uint_1431655764",
                 "%uint_2863311528 = OpConstant %uint 2863311528",
-                "%uint_2863311528_0 = OpConstant %uint 2863311528",
                 "%spec_v2uint_imul = OpConstantComposite %v2uint %uint_2863311528 %uint_2863311528",
-                "%uint_2863311528_1 = OpConstant %uint 2863311528",
-                "%uint_2863311528_2 = OpConstant %uint 2863311528",
                 "%spec_v2uint_isub_null = OpConstantComposite %v2uint %uint_2863311528 %uint_2863311528",
               },
             },
@@ -1655,34 +1583,17 @@ INSTANTIATE_TEST_SUITE_P(
                 "%v2int_minus_3 = OpConstantComposite %v2int %int_minus_3 %int_minus_3",
 
                 // srem
-                "%int_1 = OpConstant %int 1",
-                "%int_1_0 = OpConstant %int 1",
                 "%7_srem_3 = OpConstantComposite %v2int %signed_one %signed_one",
-                "%int_n1 = OpConstant %int -1",
-                "%int_n1_0 = OpConstant %int -1",
                 "%minus_7_srem_3 = OpConstantComposite %v2int %signed_minus_one %signed_minus_one",
-                "%int_1_1 = OpConstant %int 1",
-                "%int_1_2 = OpConstant %int 1",
                 "%7_srem_minus_3 = OpConstantComposite %v2int %signed_one %signed_one",
-                "%int_n1_1 = OpConstant %int -1",
-                "%int_n1_2 = OpConstant %int -1",
                 "%minus_7_srem_minus_3 = OpConstantComposite %v2int %signed_minus_one %signed_minus_one",
                 // smod
-                "%int_1_3 = OpConstant %int 1",
-                "%int_1_4 = OpConstant %int 1",
                 "%7_smod_3 = OpConstantComposite %v2int %signed_one %signed_one",
-                "%int_2 = OpConstant %int 2",
-                "%int_2_0 = OpConstant %int 2",
                 "%minus_7_smod_3 = OpConstantComposite %v2int %signed_two %signed_two",
                 "%int_n2 = OpConstant %int -2",
-                "%int_n2_0 = OpConstant %int -2",
                 "%7_smod_minus_3 = OpConstantComposite %v2int %int_n2 %int_n2",
-                "%int_n1_3 = OpConstant %int -1",
-                "%int_n1_4 = OpConstant %int -1",
                 "%minus_7_smod_minus_3 = OpConstantComposite %v2int %signed_minus_one %signed_minus_one",
                 // umod
-                "%uint_1 = OpConstant %uint 1",
-                "%uint_1_0 = OpConstant %uint 1",
                 "%7_umod_3 = OpConstantComposite %v2uint %unsigned_one %unsigned_one",
               },
             },
@@ -1702,26 +1613,15 @@ INSTANTIATE_TEST_SUITE_P(
               },
               // expected
               {
-                "%int_2 = OpConstant %int 2",
-                "%int_2_0 = OpConstant %int 2",
                 "%xor_1_3 = OpConstantComposite %v2int %signed_two %signed_two",
-                "%int_0 = OpConstant %int 0",
-                "%int_0_0 = OpConstant %int 0",
                 "%and_1_2 = OpConstantComposite %v2int %signed_zero %signed_zero",
-                "%int_3 = OpConstant %int 3",
-                "%int_3_0 = OpConstant %int 3",
                 "%or_1_2 = OpConstantComposite %v2int %signed_three %signed_three",
 
                 "%unsigned_31 = OpConstant %uint 31",
                 "%v2unsigned_31 = OpConstantComposite %v2uint %unsigned_31 %unsigned_31",
                 "%uint_2147483648 = OpConstant %uint 2147483648",
-                "%uint_2147483648_0 = OpConstant %uint 2147483648",
                 "%unsigned_left_shift_max = OpConstantComposite %v2uint %uint_2147483648 %uint_2147483648",
-                "%uint_1 = OpConstant %uint 1",
-                "%uint_1_0 = OpConstant %uint 1",
                 "%unsigned_right_shift_logical = OpConstantComposite %v2uint %unsigned_one %unsigned_one",
-                "%int_n1 = OpConstant %int -1",
-                "%int_n1_0 = OpConstant %int -1",
                 "%signed_right_shift_arithmetic = OpConstantComposite %v2int %signed_minus_one %signed_minus_one",
               },
             },
@@ -2091,7 +1991,6 @@ INSTANTIATE_TEST_SUITE_P(
               "%spec_int_20 = OpConstant %int 101",
               "%used_vec_a = OpConstantComposite %v2int %spec_int_18 %spec_int_19",
               "%int_10201 = OpConstant %int 10201",
-              "%int_1 = OpConstant %int 1",
               "%used_vec_b = OpConstantComposite %v2int %int_10201 %signed_one",
               "%spec_int_21 = OpConstant %int 10201",
               "%array = OpConstantComposite %type_arr_int_4 %spec_int_20 %spec_int_20 %spec_int_21 %spec_int_21",