Support OpPhi when replacing boolean constant operand (#3518)

Fixes #2902.

source/fuzz/fuzzer_pass_obfuscate_constants.cpp

@@ -477,28 +477,18 @@ void FuzzerPassObfuscateConstants::Apply() {
           skipped_opcode_count.clear();
         }
 
-        switch (inst.opcode()) {
+        // The instruction must not be an OpVariable, the only id that an
-          case SpvOpPhi:
+        // OpVariable uses is an initializer id, which has to remain
-            // The instruction must not be an OpPhi, as we cannot insert
+        // constant.
-            // instructions before an OpPhi.
+        if (inst.opcode() != SpvOpVariable) {
-            // TODO(https://github.com/KhronosGroup/SPIRV-Tools/issues/2902):
+          // Consider each operand of the instruction, and add a constant id
-            //  there is scope for being less conservative.
+          // use for the operand if relevant.
-            break;
+          for (uint32_t in_operand_index = 0;
-          case SpvOpVariable:
+               in_operand_index < inst.NumInOperands(); in_operand_index++) {
-            // The instruction must not be an OpVariable, the only id that an
+            MaybeAddConstantIdUse(inst, in_operand_index,
-            // OpVariable uses is an initializer id, which has to remain
+                                  base_instruction_result_id,
-            // constant.
+                                  skipped_opcode_count, &constant_uses);
-            break;
+          }
-          default:
-            // Consider each operand of the instruction, and add a constant id
-            // use for the operand if relevant.
-            for (uint32_t in_operand_index = 0;
-                 in_operand_index < inst.NumInOperands(); in_operand_index++) {
-              MaybeAddConstantIdUse(inst, in_operand_index,
-                                    base_instruction_result_id,
-                                    skipped_opcode_count, &constant_uses);
-            }
-            break;
         }
 
         if (!inst.HasResultId()) {

source/fuzz/transformation_replace_boolean_constant_with_constant_binary.cpp

@@ -243,22 +243,15 @@ bool TransformationReplaceBooleanConstantWithConstantBinary::IsApplicable(
     return false;
   }
 
-  switch (instruction->opcode()) {
+  // The instruction must not be an OpVariable, because (a) we cannot insert
-    case SpvOpPhi:
+  // a binary operator before an OpVariable, but in any case (b) the
-      // The instruction must not be an OpPhi, as we cannot insert a binary
+  // constant we would be replacing is the initializer constant of the
-      // operator instruction before an OpPhi.
+  // OpVariable, and this cannot be the result of a binary operation.
-      // TODO(https://github.com/KhronosGroup/SPIRV-Tools/issues/2902): there is
+  if (instruction->opcode() == SpvOpVariable) {
-      //  scope for being less conservative.
+    return false;
-      return false;
-    case SpvOpVariable:
-      // The instruction must not be an OpVariable, because (a) we cannot insert
-      // a binary operator before an OpVariable, but in any case (b) the
-      // constant we would be replacing is the initializer constant of the
-      // OpVariable, and this cannot be the result of a binary operation.
-      return false;
-    default:
-      return true;
   }
+
+  return true;
 }
 
 void TransformationReplaceBooleanConstantWithConstantBinary::Apply(
@@ -281,11 +274,22 @@ TransformationReplaceBooleanConstantWithConstantBinary::ApplyWithResult(
   opt::Instruction* result = binary_instruction.get();
   auto instruction_containing_constant_use =
       FindInstructionContainingUse(message_.id_use_descriptor(), ir_context);
+  auto instruction_before_which_to_insert = instruction_containing_constant_use;
+
+  // If |instruction_before_which_to_insert| is an OpPhi instruction,
+  // then |binary_instruction| will be inserted into the parent block associated
+  // with the OpPhi variable operand.
+  if (instruction_containing_constant_use->opcode() == SpvOpPhi) {
+    instruction_before_which_to_insert =
+        ir_context->cfg()
+            ->block(instruction_containing_constant_use->GetSingleWordInOperand(
+                message_.id_use_descriptor().in_operand_index() + 1))
+            ->terminator();
+  }
 
   // We want to insert the new instruction before the instruction that contains
   // the use of the boolean, but we need to go backwards one more instruction if
   // the using instruction is preceded by a merge instruction.
-  auto instruction_before_which_to_insert = instruction_containing_constant_use;
   {
     opt::Instruction* previous_node =
         instruction_before_which_to_insert->PreviousNode();
@@ -294,6 +298,7 @@ TransformationReplaceBooleanConstantWithConstantBinary::ApplyWithResult(
       instruction_before_which_to_insert = previous_node;
     }
   }
+
   instruction_before_which_to_insert->InsertBefore(
       std::move(binary_instruction));
   instruction_containing_constant_use->SetInOperand(

test/fuzz/transformation_replace_boolean_constant_with_constant_binary_test.cpp

@@ -616,41 +616,41 @@ TEST(TransformationReplaceBooleanConstantWithConstantBinaryTest, OpPhi) {
   // Hand-written SPIR-V to check applicability of the transformation on an
   // OpPhi argument.
 
-  std::string shader = R"(
+  std::string reference_shader = R"(
                OpCapability Shader
           %1 = OpExtInstImport "GLSL.std.450"
                OpMemoryModel Logical GLSL450
-               OpEntryPoint Fragment %4 "main"
+               OpEntryPoint Vertex %10 "main"
-               OpExecutionMode %4 OriginUpperLeft
+
-               OpSource ESSL 310
+; Types
-               OpName %4 "main"
           %2 = OpTypeVoid
           %3 = OpTypeFunction %2
-          %6 = OpTypeBool
+          %4 = OpTypeInt 32 0
-          %7 = OpTypePointer Function %6
+          %5 = OpTypeBool
-          %9 = OpConstantTrue %6
+
-         %16 = OpConstantFalse %6
+; Constants
-         %10 = OpTypeInt 32 1
+          %6 = OpConstant %4 0
-         %11 = OpTypePointer Function %10
+          %7 = OpConstant %4 1
-         %13 = OpConstant %10 0
+          %8 = OpConstantTrue %5
-         %15 = OpConstant %10 1
+          %9 = OpConstantFalse %5
-          %4 = OpFunction %2 None %3
+
-          %5 = OpLabel
+; main function
-               OpSelectionMerge %20 None
+         %10 = OpFunction %2 None %3
-               OpBranchConditional %9 %21 %22
+         %11 = OpLabel
-         %21 = OpLabel
+               OpSelectionMerge %13 None
-               OpBranch %20
+               OpBranchConditional %8 %12 %13
-         %22 = OpLabel
+         %12 = OpLabel
-               OpBranch %20
+               OpBranch %13
-         %20 = OpLabel
+         %13 = OpLabel
-         %23 = OpPhi %6 %9 %21 %16 %22
+         %14 = OpPhi %5 %8 %11 %9 %12
                OpReturn
                OpFunctionEnd
   )";
 
   const auto env = SPV_ENV_UNIVERSAL_1_3;
   const auto consumer = nullptr;
-  const auto context = BuildModule(env, consumer, shader, kFuzzAssembleOption);
+  const auto context =
+      BuildModule(env, consumer, reference_shader, kFuzzAssembleOption);
   ASSERT_TRUE(IsValid(env, context.get()));
 
   FactManager fact_manager;
@@ -658,11 +658,48 @@ TEST(TransformationReplaceBooleanConstantWithConstantBinaryTest, OpPhi) {
   TransformationContext transformation_context(&fact_manager,
                                                validator_options);
 
-  auto replacement = TransformationReplaceBooleanConstantWithConstantBinary(
+  auto instruction_descriptor = MakeInstructionDescriptor(14, SpvOpPhi, 0);
-      MakeIdUseDescriptor(9, MakeInstructionDescriptor(23, SpvOpPhi, 0), 0), 13,
+  auto id_use_descriptor = MakeIdUseDescriptor(8, instruction_descriptor, 0);
-      15, SpvOpSLessThan, 100);
+  auto transformation = TransformationReplaceBooleanConstantWithConstantBinary(
+      id_use_descriptor, 6, 7, SpvOpULessThan, 15);
+  ASSERT_TRUE(
+      transformation.IsApplicable(context.get(), transformation_context));
+  transformation.Apply(context.get(), &transformation_context);
 
-  ASSERT_FALSE(replacement.IsApplicable(context.get(), transformation_context));
+  std::string variant_shader = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Vertex %10 "main"
+
+; Types
+          %2 = OpTypeVoid
+          %3 = OpTypeFunction %2
+          %4 = OpTypeInt 32 0
+          %5 = OpTypeBool
+
+; Constants
+          %6 = OpConstant %4 0
+          %7 = OpConstant %4 1
+          %8 = OpConstantTrue %5
+          %9 = OpConstantFalse %5
+
+; main function
+         %10 = OpFunction %2 None %3
+         %11 = OpLabel
+         %15 = OpULessThan %5 %6 %7
+               OpSelectionMerge %13 None
+               OpBranchConditional %8 %12 %13
+         %12 = OpLabel
+               OpBranch %13
+         %13 = OpLabel
+         %14 = OpPhi %5 %15 %11 %9 %12
+               OpReturn
+               OpFunctionEnd
+  )";
+
+  ASSERT_TRUE(IsValid(env, context.get()));
+  ASSERT_TRUE(IsEqual(env, variant_shader, context.get()));
 }
 
 TEST(TransformationReplaceBooleanConstantWithConstantBinaryTest,