spirv-fuzz: Fixes to preconditions for adding dead break/continue edges (#2904)

Issues #2898 and #2900 identify some cases where adding a dead
continue would lead to an invalid module, and these turned out to be
due to the lack of sensible dominance information when a continue
target is unreachable. This change requires that the header of a loop
dominates the loop's continue target if a dead continue is to be
added.

Furthermore, issue #2905 identified a shortcoming in the algorithm
being used to identify when it is OK, from a dominance point of view,
to add a new break/continue edge to a control flow graph. This change
replaces that algorithm with a simpler and more obviously correct
algorithm (that incidentally does not require the new edge to be a
break/continue edge in particular).

Fixes #2898.
Fixes #2900.
Fixes #2905.

source/fuzz/fuzzer_util.cpp

@@ -205,156 +205,71 @@ opt::BasicBlock::iterator GetIteratorForBaseInstructionAndOffset(
   return nullptr;
 }
 
-// Returns the ids of all successors of |block|
-std::vector<uint32_t> GetSuccessors(opt::BasicBlock* block) {
-  std::vector<uint32_t> result;
-  switch (block->terminator()->opcode()) {
-    case SpvOpBranch:
-      result.push_back(block->terminator()->GetSingleWordInOperand(0));
-      break;
-    case SpvOpBranchConditional:
-      result.push_back(block->terminator()->GetSingleWordInOperand(1));
-      result.push_back(block->terminator()->GetSingleWordInOperand(2));
-      break;
-    case SpvOpSwitch:
-      for (uint32_t i = 1; i < block->terminator()->NumInOperands(); i += 2) {
-        result.push_back(block->terminator()->GetSingleWordInOperand(i));
-      }
-      break;
-    default:
-      break;
+bool NewEdgeRespectsUseDefDominance(opt::IRContext* context,
+                                    opt::BasicBlock* bb_from,
+                                    opt::BasicBlock* bb_to) {
+  assert(bb_from->terminator()->opcode() == SpvOpBranch);
+
+  // If there is *already* an edge from |bb_from| to |bb_to|, then adding
+  // another edge is fine from a dominance point of view.
+  if (bb_from->terminator()->GetSingleWordInOperand(0) == bb_to->id()) {
+    return true;
   }
-  return result;
-}
 
-// The FindBypassedBlocks method and its helpers perform a depth-first search;
-// this struct represents an element of the stack used during depth-first
-// search.
-struct FindBypassedBlocksDfsStackNode {
-  opt::BasicBlock* block;  // The block that is being explored
-  bool handled_merge;  // We visit merge blocks before successors; this field
-                       // tracks whether we have yet processed the merge block
-                       // (if any) associated with the block
-  uint32_t next_successor;  // The next as-yet unexplored successor of this
-                            // block; exploration of a block is complete when
-                            // this field's value reaches the successor count
-};
-
-// Helper method for the depth-first-search routine that collects blocks that a
-// new break or continue control flow graph edge will bypass.
-void HandleSuccessorDuringSearchForBypassedBlocks(
-    opt::BasicBlock* successor, bool new_blocks_will_be_bypassed,
-    std::set<uint32_t>* already_visited,
-    std::set<opt::BasicBlock*>* bypassed_blocks,
-    std::vector<FindBypassedBlocksDfsStackNode>* dfs_stack) {
-  if (already_visited->count(successor->id()) == 0) {
-    // This is a new block; mark it as visited so that we don't regard it as new
-    // in the future, and push it on to the stack for exploration.
-    already_visited->insert(successor->id());
-    dfs_stack->push_back({successor, false, 0});
-    if (new_blocks_will_be_bypassed) {
-      // We are in the region of the control-flow graph consisting of blocks
-      // that the new edge will bypass, so grab this block.
-      bypassed_blocks->insert(successor);
-    }
-  }
-}
-
-// Determines those block that will be bypassed by a break or continue edge from
-// |bb_from| to |bb_to|.
-void FindBypassedBlocks(opt::IRContext* context, opt::BasicBlock* bb_from,
-                        opt::BasicBlock* bb_to,
-                        std::set<opt::BasicBlock*>* bypassed_blocks) {
-  // This algorithm finds all blocks different from |bb_from| that:
-  // - are in the innermost structured control flow construct containing
-  // |bb_from|
-  // - can be reached from |bb_from| without traversing a back-edge or going
-  // through |bb_to|
+  // Let us assume that the module being manipulated is valid according to the
+  // rules of the SPIR-V language.
   //
-  // This is achieved by doing a depth-first search of the function's CFG,
-  // exploring merge blocks before successors, and grabbing all blocks that are
-  // visited in the sub-search rooted at |bb_from|. (As an optimization, the
-  // search terminates as soon as exploration of |bb_from| has completed.)
+  // Suppose that some block Y is dominated by |bb_to| (which includes the case
+  // where Y = |bb_to|).
+  //
+  // Suppose that Y uses an id i that is defined in some other block X.
+  //
+  // Because the module is valid, X must dominate Y.  We are concerned about
+  // whether an edge from |bb_from| to |bb_to| could *stop* X from dominating
+  // Y.
+  //
+  // Because |bb_to| dominates Y, a new edge from |bb_from| to |bb_to| can
+  // only affect whether X dominates Y if X dominates |bb_to|.
+  //
+  // So let us assume that X does dominate |bb_to|, so that we have:
+  //
+  //   (X defines i) dominates |bb_to| dominates (Y uses i)
+  //
+  // The new edge from |bb_from| to |bb_to| will stop the definition of i in X
+  // from dominating the use of i in Y exactly when the new edge will stop X
+  // from dominating |bb_to|.
+  //
+  // Now, the block X that we are worried about cannot dominate |bb_from|,
+  // because in that case X would still dominate |bb_to| after we add an edge
+  // from |bb_from| to |bb_to|.
+  //
+  // Also, it cannot be that X = |bb_to|, because nothing can stop a block
+  // from dominating itself.
+  //
+  // So we are looking for a block X such that:
+  //
+  // - X strictly dominates |bb_to|
+  // - X does not dominate |bb_from|
+  // - X defines an id i
+  // - i is used in some block Y
+  // - |bb_to| dominates Y
 
-  auto enclosing_function = bb_from->GetParent();
-
-  // The set of block ids already visited during search.  We put |bb_to| in
-  // there initially so that search automatically backtracks when this block is
-  // reached.
-  std::set<uint32_t> already_visited;
-  already_visited.insert(bb_to->id());
-
-  // Tracks when we are in the region of blocks that the new edge would bypass;
-  // we flip this to 'true' once we reach |bb_from| and have finished searching
-  // its merge block (in the case that it happens to be a header.
-  bool new_blocks_will_be_bypassed = false;
-
-  std::vector<FindBypassedBlocksDfsStackNode> dfs_stack;
-  opt::BasicBlock* entry_block = enclosing_function->entry().get();
-  dfs_stack.push_back({entry_block, false, 0});
-  while (!dfs_stack.empty()) {
-    auto node_index = dfs_stack.size() - 1;
-
-    // First make sure we search the merge block associated ith this block, if
-    // there is one.
-    if (!dfs_stack[node_index].handled_merge) {
-      dfs_stack[node_index].handled_merge = true;
-      if (dfs_stack[node_index].block->MergeBlockIdIfAny()) {
-        opt::BasicBlock* merge_block = context->cfg()->block(
-            dfs_stack[node_index].block->MergeBlockIdIfAny());
-        // A block can only be the merge block for one header, so this block
-        // should only be in |visited| if it is |bb_to|, which we put into
-        // |visited| in advance.
-        assert(already_visited.count(merge_block->id()) == 0 ||
-               merge_block == bb_to);
-        HandleSuccessorDuringSearchForBypassedBlocks(
-            merge_block, new_blocks_will_be_bypassed, &already_visited,
-            bypassed_blocks, &dfs_stack);
-      }
-      continue;
-    }
-
-    // If we find |bb_from|, we are interested in grabbing previously unseen
-    // successor blocks (by this point we will have already searched the merge
-    // block associated with |bb_from|, if there is one.
-    if (dfs_stack[node_index].block == bb_from) {
-      new_blocks_will_be_bypassed = true;
-    }
-
-    // Consider the next unexplored successor.
-    auto successors = GetSuccessors(dfs_stack[node_index].block);
-    if (dfs_stack[node_index].next_successor < successors.size()) {
-      HandleSuccessorDuringSearchForBypassedBlocks(
-          context->cfg()->block(
-              successors[dfs_stack[node_index].next_successor]),
-          new_blocks_will_be_bypassed, &already_visited, bypassed_blocks,
-          &dfs_stack);
-      dfs_stack[node_index].next_successor++;
-    } else {
-      // We have finished exploring |node|.  If it is |bb_from|, we can
-      // terminate search -- we have grabbed all the relevant blocks.
-      if (dfs_stack[node_index].block == bb_from) {
-        break;
-      }
-      dfs_stack.pop_back();
-    }
-  }
-}
-
-bool NewEdgeLeavingConstructBodyRespectsUseDefDominance(
-    opt::IRContext* context, opt::BasicBlock* bb_from, opt::BasicBlock* bb_to) {
-  // Find those blocks that the edge from |bb_from| to |bb_to| might bypass.
-  std::set<opt::BasicBlock*> bypassed_blocks;
-  FindBypassedBlocks(context, bb_from, bb_to, &bypassed_blocks);
-
-  // For each bypassed block, check whether it contains a definition that is
-  // used by some non-bypassed block - that would be problematic.
-  for (auto defining_block : bypassed_blocks) {
-    for (auto& inst : *defining_block) {
+  // Walk the dominator tree backwards, starting from the immediate dominator
+  // of |bb_to|.  We can stop when we find a block that also dominates
+  // |bb_from|.
+  auto dominator_analysis = context->GetDominatorAnalysis(bb_from->GetParent());
+  for (auto dominator = dominator_analysis->ImmediateDominator(bb_to);
+       dominator != nullptr &&
+       !dominator_analysis->Dominates(dominator, bb_from);
+       dominator = dominator_analysis->ImmediateDominator(dominator)) {
+    // |dominator| is a candidate for block X in the above description.
+    // We now look through the instructions for a candidate instruction i.
+    for (auto& inst : *dominator) {
+      // Consider all the uses of this instruction.
       if (!context->get_def_use_mgr()->WhileEachUse(
               &inst,
-              [context, &bypassed_blocks](opt::Instruction* user,
-                                          uint32_t operand_index) -> bool {
+              [bb_to, context, dominator_analysis](
+                  opt::Instruction* user, uint32_t operand_index) -> bool {
                 // If this use is in an OpPhi, we need to check that dominance
                 // of the relevant *parent* block is not spoiled.  Otherwise we
                 // need to check that dominance of the block containing the use
@@ -371,13 +286,12 @@ bool NewEdgeLeavingConstructBodyRespectsUseDefDominance(
                   return true;
                 }
 
-                // If the use-block is not in |bypassed_blocks| then we have
-                // found a block in the construct that is reachable from
-                // |from_block|, and which defines an id that is used outside of
-                // the construct.  Adding an edge from |from_block| to
-                // |to_block| would prevent this use being dominated.
-                return bypassed_blocks.find(use_block_or_phi_parent) !=
-                       bypassed_blocks.end();
+                // With reference to the above discussion,
+                // |use_block_or_phi_parent| is a candidate for the block Y.
+                // If |bb_to| dominates this block, the new edge would be
+                // problematic.
+                return !dominator_analysis->Dominates(bb_to,
+                                                      use_block_or_phi_parent);
               })) {
         return false;
       }

source/fuzz/fuzzer_util.h

@@ -74,18 +74,13 @@ bool BlockIsInLoopContinueConstruct(opt::IRContext* context, uint32_t block_id,
 opt::BasicBlock::iterator GetIteratorForBaseInstructionAndOffset(
     opt::BasicBlock* block, const opt::Instruction* base_inst, uint32_t offset);
 
-// Block |bb_from| is assumed to be in a structured control flow construct, and
-// block |bb_to| is assumed to be either the merge bock for that construct (in
-// the case of a loop, conditional or switch) or the continue target for that
-// construct (in the case of a loop only).
-//
 // The function determines whether adding an edge from |bb_from| to |bb_to| -
-// i.e. a break or continue for the construct
-// - is legitimate with respect to the SPIR-V rule that a definition must
+// is legitimate with respect to the SPIR-V rule that a definition must
 // dominate all of its uses.  This is because adding such an edge can change
 // dominance in the control flow graph, potentially making the module invalid.
-bool NewEdgeLeavingConstructBodyRespectsUseDefDominance(
-    opt::IRContext* context, opt::BasicBlock* bb_from, opt::BasicBlock* bb_to);
+bool NewEdgeRespectsUseDefDominance(opt::IRContext* context,
+                                    opt::BasicBlock* bb_from,
+                                    opt::BasicBlock* bb_to);
 
 }  // namespace fuzzerutil
 

source/fuzz/transformation_add_dead_break.cpp

@@ -170,8 +170,7 @@ bool TransformationAddDeadBreak::IsApplicable(
 
   // Check that adding the break would not violate the property that a
   // definition must dominate all of its uses.
-  return fuzzerutil::NewEdgeLeavingConstructBodyRespectsUseDefDominance(
-      context, bb_from, bb_to);
+  return fuzzerutil::NewEdgeRespectsUseDefDominance(context, bb_from, bb_to);
 }
 
 void TransformationAddDeadBreak::Apply(opt::IRContext* context,

source/fuzz/transformation_add_dead_continue.cpp

@@ -83,14 +83,23 @@ bool TransformationAddDeadContinue::IsApplicable(
     return false;
   }
 
+  auto continue_block = context->cfg()->block(loop_header)->ContinueBlockId();
+
+  if (!context->GetDominatorAnalysis(bb_from->GetParent())
+           ->Dominates(loop_header, continue_block)) {
+    // If the loop's continue block is not dominated by the loop header, the
+    // continue block is unreachable. In that case, we conservatively do not
+    // allow adding a dead continue, to avoid the compilations that arise due to
+    // the lack of sensible dominance information for unreachable blocks.
+    return false;
+  }
+
   if (fuzzerutil::BlockIsInLoopContinueConstruct(context, message_.from_block(),
                                                  loop_header)) {
     // We cannot jump to the continue target from the continue construct.
     return false;
   }
 
-  auto continue_block = context->cfg()->block(loop_header)->ContinueBlockId();
-
   if (context->GetStructuredCFGAnalysis()->IsMergeBlock(continue_block)) {
     // A branch straight to the continue target that is also a merge block might
     // break the property that a construct header must dominate its merge block
@@ -100,7 +109,7 @@ bool TransformationAddDeadContinue::IsApplicable(
 
   // Check that adding the continue would not violate the property that a
   // definition must dominate all of its uses.
-  if (!fuzzerutil::NewEdgeLeavingConstructBodyRespectsUseDefDominance(
+  if (!fuzzerutil::NewEdgeRespectsUseDefDominance(
           context, bb_from, context->cfg()->block(continue_block))) {
     return false;
   }

source/fuzz/transformation_add_dead_continue.h

@@ -36,6 +36,8 @@ class TransformationAddDeadContinue : public Transformation {
 
   // - |message_.from_block| must be the id of a block a in the given module.
   // - a must be contained in a loop with continue target b
+  // - The continue target b must be dominated by the head of the loop in which
+  //   it is contained
   // - b must not be the merge block of a selection construct
   // - if |message_.continue_condition_value| holds (does not hold) then
   //   OpConstantTrue (OpConstantFalse) must be present in the module

test/fuzz/transformation_add_dead_continue_test.cpp

@@ -1388,6 +1388,244 @@ TEST(TransformationAddDeadContinueTest, Miscellaneous1) {
   ASSERT_FALSE(bad_transformation.IsApplicable(context.get(), fact_manager));
 }
 
+TEST(TransformationAddDeadContinueTest, Miscellaneous2) {
+  // A miscellaneous test that exposed a bug in spirv-fuzz.
+
+  std::string shader = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %4 "main"
+               OpExecutionMode %4 OriginUpperLeft
+               OpSource ESSL 310
+          %2 = OpTypeVoid
+          %3 = OpTypeFunction %2
+         %51 = OpTypeBool
+        %395 = OpConstantTrue %51
+          %4 = OpFunction %2 None %3
+          %5 = OpLabel
+               OpBranch %389
+        %389 = OpLabel
+               OpLoopMerge %388 %391 None
+               OpBranch %339
+        %339 = OpLabel
+               OpSelectionMerge %396 None
+               OpBranchConditional %395 %388 %396
+        %396 = OpLabel
+               OpBranch %1552
+       %1552 = OpLabel
+               OpLoopMerge %1553 %1554 None
+               OpBranch %1556
+       %1556 = OpLabel
+               OpLoopMerge %1557 %1570 None
+               OpBranchConditional %395 %1562 %1557
+       %1562 = OpLabel
+               OpSelectionMerge %1570 None
+               OpBranchConditional %395 %1571 %1570
+       %1571 = OpLabel
+               OpBranch %1557
+       %1570 = OpLabel
+               OpBranch %1556
+       %1557 = OpLabel
+               OpSelectionMerge %1586 None
+               OpBranchConditional %395 %1553 %1586
+       %1586 = OpLabel
+               OpBranch %1553
+       %1554 = OpLabel
+               OpBranch %1552
+       %1553 = OpLabel
+               OpBranch %388
+        %391 = OpLabel
+               OpBranch %389
+        %388 = OpLabel
+               OpReturn
+               OpFunctionEnd
+  )";
+
+  const auto env = SPV_ENV_UNIVERSAL_1_3;
+  const auto consumer = nullptr;
+  const auto context = BuildModule(env, consumer, shader, kFuzzAssembleOption);
+  ASSERT_TRUE(IsValid(env, context.get()));
+
+  FactManager fact_manager;
+
+  // This transformation would introduce a branch from a continue target to
+  // itself.
+  auto bad_transformation = TransformationAddDeadContinue(1554, true, {});
+  ASSERT_FALSE(bad_transformation.IsApplicable(context.get(), fact_manager));
+}
+
+TEST(TransformationAddDeadContinueTest, Miscellaneous3) {
+  // A miscellaneous test that exposed a bug in spirv-fuzz.
+
+  std::string shader = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %4 "main"
+               OpExecutionMode %4 OriginUpperLeft
+               OpSource ESSL 310
+          %2 = OpTypeVoid
+          %3 = OpTypeFunction %2
+         %85 = OpTypeBool
+        %434 = OpConstantFalse %85
+          %4 = OpFunction %2 None %3
+          %5 = OpLabel
+               OpBranch %234
+        %234 = OpLabel
+               OpLoopMerge %235 %236 None
+               OpBranch %259
+        %259 = OpLabel
+               OpLoopMerge %260 %274 None
+               OpBranchConditional %434 %265 %260
+        %265 = OpLabel
+               OpBranch %275
+        %275 = OpLabel
+               OpBranch %260
+        %274 = OpLabel
+               OpBranch %259
+        %260 = OpLabel
+               OpSelectionMerge %298 None
+               OpBranchConditional %434 %299 %300
+        %300 = OpLabel
+               OpBranch %235
+        %298 = OpLabel
+               OpUnreachable
+        %236 = OpLabel
+               OpBranch %234
+        %299 = OpLabel
+               OpBranch %235
+        %235 = OpLabel
+               OpReturn
+               OpFunctionEnd
+  )";
+
+  const auto env = SPV_ENV_UNIVERSAL_1_3;
+  const auto consumer = nullptr;
+  const auto context = BuildModule(env, consumer, shader, kFuzzAssembleOption);
+  ASSERT_TRUE(IsValid(env, context.get()));
+
+  FactManager fact_manager;
+
+  auto bad_transformation = TransformationAddDeadContinue(299, false, {});
+
+  // The continue edge would connect %299 to the previously-unreachable %236,
+  // making %299 dominate %236, and breaking the rule that block ordering must
+  // respect dominance.
+  ASSERT_FALSE(bad_transformation.IsApplicable(context.get(), fact_manager));
+}
+
+TEST(TransformationAddDeadContinueTest, Miscellaneous4) {
+  // A miscellaneous test that exposed a bug in spirv-fuzz.
+
+  std::string shader = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %4 "main"
+               OpExecutionMode %4 OriginUpperLeft
+               OpSource ESSL 310
+               OpName %4 "main"
+               OpName %8 "i"
+          %2 = OpTypeVoid
+          %3 = OpTypeFunction %2
+          %6 = OpTypeInt 32 1
+          %7 = OpTypePointer Function %6
+          %9 = OpConstant %6 0
+         %16 = OpConstant %6 100
+         %17 = OpTypeBool
+        %100 = OpConstantFalse %17
+         %21 = OpConstant %6 1
+          %4 = OpFunction %2 None %3
+          %5 = OpLabel
+          %8 = OpVariable %7 Function
+               OpStore %8 %9
+               OpBranch %10
+         %13 = OpLabel
+         %20 = OpLoad %6 %8
+         %22 = OpIAdd %6 %20 %21
+               OpStore %8 %22
+               OpBranch %10
+         %10 = OpLabel
+               OpLoopMerge %12 %13 None
+               OpBranch %14
+         %14 = OpLabel
+         %15 = OpLoad %6 %8
+         %18 = OpSLessThan %17 %15 %16
+               OpBranchConditional %18 %11 %12
+         %11 = OpLabel
+               OpBranch %12
+         %12 = OpLabel
+               OpReturn
+               OpFunctionEnd
+  )";
+
+  const auto env = SPV_ENV_UNIVERSAL_1_3;
+  const auto consumer = nullptr;
+  const auto context = BuildModule(env, consumer, shader, kFuzzAssembleOption);
+  ASSERT_TRUE(IsValid(env, context.get()));
+
+  FactManager fact_manager;
+
+  auto bad_transformation = TransformationAddDeadContinue(10, false, {});
+
+  // The continue edge would connect %10 to the previously-unreachable %13,
+  // making %10 dominate %13, and breaking the rule that block ordering must
+  // respect dominance.
+  ASSERT_FALSE(bad_transformation.IsApplicable(context.get(), fact_manager));
+}
+
+TEST(TransformationAddDeadContinueTest, Miscellaneous5) {
+  // A miscellaneous test that exposed a bug in spirv-fuzz.
+
+  std::string shader = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %4 "main"
+               OpExecutionMode %4 OriginUpperLeft
+               OpSource ESSL 310
+          %2 = OpTypeVoid
+          %3 = OpTypeFunction %2
+          %6 = OpTypeBool
+          %7 = OpTypePointer Function %6
+          %9 = OpConstantTrue %6
+          %4 = OpFunction %2 None %3
+          %5 = OpLabel
+               OpBranch %98
+         %98 = OpLabel
+               OpLoopMerge %100 %101 None
+               OpBranch %99
+         %99 = OpLabel
+               OpSelectionMerge %111 None
+               OpBranchConditional %9 %110 %111
+        %110 = OpLabel
+               OpBranch %100
+        %111 = OpLabel
+        %200 = OpCopyObject %6 %9
+               OpBranch %101
+        %101 = OpLabel
+        %201 = OpCopyObject %6 %200
+               OpBranchConditional %9 %98 %100
+        %100 = OpLabel
+               OpReturn
+               OpFunctionEnd
+  )";
+
+  const auto env = SPV_ENV_UNIVERSAL_1_3;
+  const auto consumer = nullptr;
+  const auto context = BuildModule(env, consumer, shader, kFuzzAssembleOption);
+  ASSERT_TRUE(IsValid(env, context.get()));
+
+  FactManager fact_manager;
+
+  auto bad_transformation = TransformationAddDeadContinue(110, true, {});
+
+  // The continue edge would lead to the use of %200 in block %101 no longer
+  // being dominated by its definition in block %111.
+  ASSERT_FALSE(bad_transformation.IsApplicable(context.get(), fact_manager));
+}
+
 }  // namespace
 }  // namespace fuzz
 }  // namespace spvtools