spirv-opt: Fix IRContext analysis handling (KhronosGroup#6765)

s-perron · web-flow · commit 85f222ec5918 · 2026-07-10T12:28:32.000-04:00
1. Add more cases where invalidating on analysis imply invalidating
others.
2. Remove early return limiting the consistenty checks. Tests are added
   to catch issues like this again.
3. Extend IRContext::IsConsistent() to verify kAnalysisDominatorAnalysis
and
   kAnalysisStructuredCFG analyses.

Assisted by Antigravity
diff --git a/source/opt/dominator_analysis.h b/source/opt/dominator_analysis.h
@@ -116,6 +116,11 @@ class DominatorAnalysisBase {
   // If there is no such basic block, nullptr is returned.
   BasicBlock* CommonDominator(BasicBlock* b1, BasicBlock* b2) const;
 
+  // Returns true if this analysis has the same tree as |other|.
+  inline bool operator==(const DominatorAnalysisBase& other) const {
+    return tree_ == other.tree_;
+  }
+
  protected:
   DominatorTree tree_;
 };
diff --git a/source/opt/dominator_tree.cpp b/source/opt/dominator_tree.cpp
@@ -381,5 +381,24 @@ void DominatorTree::DumpTreeAsDot(std::ostream& out_stream) const {
   out_stream << "}\n";
 }
 
+bool DominatorTree::operator==(const DominatorTree& other) const {
+  if (postdominator_ != other.postdominator_) return false;
+  if (nodes_.size() != other.nodes_.size()) return false;
+
+  for (const auto& pair : nodes_) {
+    uint32_t id = pair.first;
+    const DominatorTreeNode& node = pair.second;
+    const DominatorTreeNode* other_node = other.GetTreeNode(id);
+    if (!other_node) return false;
+
+    if ((node.parent_ == nullptr) != (other_node->parent_ == nullptr))
+      return false;
+    if (node.parent_ != nullptr) {
+      if (node.parent_->id() != other_node->parent_->id()) return false;
+    }
+  }
+  return true;
+}
+
 }  // namespace opt
 }  // namespace spvtools
diff --git a/source/opt/dominator_tree.h b/source/opt/dominator_tree.h
@@ -202,6 +202,9 @@ class DominatorTree {
   // Returns true if this tree is a post dominator tree.
   bool IsPostDominator() const { return postdominator_; }
 
+  // Returns true if this tree has the same nodes and structure as |other|.
+  bool operator==(const DominatorTree& other) const;
+
   // Clean up the tree.
   void ClearTree() {
     nodes_.clear();
diff --git a/source/opt/ir_context.cpp b/source/opt/ir_context.cpp
@@ -118,6 +118,12 @@ void IRContext::InvalidateAnalyses(IRContext::Analysis analyses_to_invalidate) {
   // dominator analysis should be invalidated as well.
   if (analyses_to_invalidate & kAnalysisCFG) {
     analyses_to_invalidate |= kAnalysisDominatorAnalysis;
+    analyses_to_invalidate |= kAnalysisStructuredCFG;
+  }
+
+  if (analyses_to_invalidate & kAnalysisLoopAnalysis) {
+    analyses_to_invalidate |= kAnalysisScalarEvolution;
+    analyses_to_invalidate |= kAnalysisLiveness;
   }
 
   if (analyses_to_invalidate & kAnalysisDefUse) {
@@ -142,6 +148,9 @@ void IRContext::InvalidateAnalyses(IRContext::Analysis analyses_to_invalidate) {
     dominator_trees_.clear();
     post_dominator_trees_.clear();
   }
+  if (analyses_to_invalidate & kAnalysisLoopAnalysis) {
+    loop_descriptors_.clear();
+  }
   if (analyses_to_invalidate & kAnalysisNameMap) {
     id_to_name_.reset(nullptr);
   }
@@ -160,6 +169,12 @@ void IRContext::InvalidateAnalyses(IRContext::Analysis analyses_to_invalidate) {
   if (analyses_to_invalidate & kAnalysisLiveness) {
     liveness_mgr_.reset(nullptr);
   }
+  if (analyses_to_invalidate & kAnalysisScalarEvolution) {
+    scalar_evolution_analysis_.reset(nullptr);
+  }
+  if (analyses_to_invalidate & kAnalysisRegisterPressure) {
+    reg_pressure_.reset(nullptr);
+  }
   if (analyses_to_invalidate & kAnalysisTypes) {
     type_mgr_.reset(nullptr);
   }
@@ -392,7 +407,6 @@ bool IRContext::IsConsistent() {
     }
   }
 
-  return true;
   if (AreAnalysesValid(kAnalysisIdToFuncMapping)) {
     for (auto& fn : *module_) {
       if (id_to_func_[fn.result_id()] != &fn) {
@@ -437,6 +451,37 @@ bool IRContext::IsConsistent() {
     }
   }
 
+  if (AreAnalysesValid(kAnalysisDominatorAnalysis)) {
+    for (const auto& it : dominator_trees_) {
+      const Function* f = it.first;
+      const DominatorAnalysis& cached_dom = it.second;
+      DominatorAnalysis new_dom;
+      new_dom.InitializeTree(*cfg(), f);
+
+      if (!(cached_dom == new_dom)) {
+        return false;
+      }
+    }
+    for (const auto& it : post_dominator_trees_) {
+      const Function* f = it.first;
+      const PostDominatorAnalysis& cached_post_dom = it.second;
+      PostDominatorAnalysis new_post_dom;
+      new_post_dom.InitializeTree(*cfg(), f);
+
+      if (!(cached_post_dom == new_post_dom)) {
+        return false;
+      }
+    }
+  }
+
+  if (AreAnalysesValid(kAnalysisStructuredCFG)) {
+    StructuredCFGAnalysis new_struct_cfg(this);
+    StructuredCFGAnalysis* cached_struct_cfg = struct_cfg_analysis_.get();
+    if (!(*cached_struct_cfg == new_struct_cfg)) {
+      return false;
+    }
+  }
+
   if (feature_mgr_ != nullptr) {
     FeatureManager current(grammar_);
     current.Analyze(module());
diff --git a/source/opt/struct_cfg_analysis.cpp b/source/opt/struct_cfg_analysis.cpp
@@ -245,5 +245,18 @@ StructuredCFGAnalysis::FindFuncsCalledFromContinue() {
   return called_from_continue;
 }
 
+bool StructuredCFGAnalysis::operator==(
+    const StructuredCFGAnalysis& other) const {
+  if (bb_to_construct_.size() != other.bb_to_construct_.size()) return false;
+  for (const auto& pair : bb_to_construct_) {
+    uint32_t bb_id = pair.first;
+    const ConstructInfo& info = pair.second;
+    auto it = other.bb_to_construct_.find(bb_id);
+    if (it == other.bb_to_construct_.end()) return false;
+    if (!(info == it->second)) return false;
+  }
+  return merge_blocks_ == other.merge_blocks_;
+}
+
 }  // namespace opt
 }  // namespace spvtools
diff --git a/source/opt/struct_cfg_analysis.h b/source/opt/struct_cfg_analysis.h
@@ -115,6 +115,10 @@ class StructuredCFGAnalysis {
   // Return true if |bb_id| is the merge block for a construct.
   bool IsMergeBlock(uint32_t bb_id);
 
+  // Returns true if this analysis has the same constructs and merge blocks as
+  // |other|.
+  bool operator==(const StructuredCFGAnalysis& other) const;
+
   // Returns the set of function ids that are called directly or indirectly from
   // a continue construct.
   std::unordered_set<uint32_t> FindFuncsCalledFromContinue();
@@ -141,6 +145,13 @@ class StructuredCFGAnalysis {
     uint32_t containing_loop;
     uint32_t containing_switch;
     bool in_continue;
+
+    bool operator==(const ConstructInfo& other) const {
+      return containing_construct == other.containing_construct &&
+             containing_loop == other.containing_loop &&
+             containing_switch == other.containing_switch &&
+             in_continue == other.in_continue;
+    }
   };
 
   // Populates |bb_to_construct_| with the innermost containing merge and loop
diff --git a/source/util/bit_vector.cpp b/source/util/bit_vector.cpp
@@ -78,5 +78,15 @@ std::ostream& operator<<(std::ostream& out, const BitVector& bv) {
   return out;
 }
 
+bool BitVector::operator==(const BitVector& other) const {
+  size_t max_size = std::max(bits_.size(), other.bits_.size());
+  for (size_t i = 0; i < max_size; ++i) {
+    BitContainer b1 = (i < bits_.size()) ? bits_[i] : 0;
+    BitContainer b2 = (i < other.bits_.size()) ? other.bits_[i] : 0;
+    if (b1 != b2) return false;
+  }
+  return true;
+}
+
 }  // namespace utils
 }  // namespace spvtools
diff --git a/source/util/bit_vector.h b/source/util/bit_vector.h
@@ -109,6 +109,10 @@ class BitVector {
   // |this|.  Return true if |this| changed.
   bool Or(const BitVector& that);
 
+  // Returns true if |this| and |that| have the same bits set.
+  bool operator==(const BitVector& that) const;
+  bool operator!=(const BitVector& that) const { return !(*this == that); }
+
  private:
   std::vector<BitContainer> bits_;
 };
diff --git a/test/opt/ir_context_test.cpp b/test/opt/ir_context_test.cpp
@@ -830,6 +830,194 @@ OpFunctionEnd)";
   EXPECT_FALSE(ctx->AreAnalysesValid(IRContext::kAnalysisDominatorAnalysis));
 }
 
+TEST_F(IRContextTest, InvalidateCFGInvalidatesAll) {
+  const std::string text = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+%1 = OpTypeVoid
+%2 = OpTypeFunction %1
+%3 = OpFunction %1 None %2
+%4 = OpLabel
+OpReturn
+OpFunctionEnd)";
+
+  std::unique_ptr<IRContext> ctx =
+      BuildModule(SPV_ENV_UNIVERSAL_1_1, nullptr, text,
+                  SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+
+  ctx->BuildInvalidAnalyses(
+      IRContext::kAnalysisCFG | IRContext::kAnalysisDominatorAnalysis |
+      IRContext::kAnalysisStructuredCFG | IRContext::kAnalysisLoopAnalysis |
+      IRContext::kAnalysisScalarEvolution | IRContext::kAnalysisLiveness);
+
+  ctx->InvalidateAnalyses(IRContext::kAnalysisCFG);
+  EXPECT_FALSE(ctx->AreAnalysesValid(IRContext::kAnalysisCFG));
+  EXPECT_FALSE(ctx->AreAnalysesValid(IRContext::kAnalysisDominatorAnalysis));
+  EXPECT_FALSE(ctx->AreAnalysesValid(IRContext::kAnalysisStructuredCFG));
+}
+
+TEST_F(IRContextTest, InvalidateDominatorInvalidatesDownstreamOnly) {
+  const std::string text = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+%1 = OpTypeVoid
+%2 = OpTypeFunction %1
+%3 = OpFunction %1 None %2
+%4 = OpLabel
+OpReturn
+OpFunctionEnd)";
+
+  std::unique_ptr<IRContext> ctx =
+      BuildModule(SPV_ENV_UNIVERSAL_1_1, nullptr, text,
+                  SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+
+  ctx->BuildInvalidAnalyses(
+      IRContext::kAnalysisCFG | IRContext::kAnalysisDominatorAnalysis |
+      IRContext::kAnalysisStructuredCFG | IRContext::kAnalysisLoopAnalysis |
+      IRContext::kAnalysisScalarEvolution | IRContext::kAnalysisLiveness);
+
+  ctx->InvalidateAnalyses(IRContext::kAnalysisDominatorAnalysis);
+  EXPECT_TRUE(ctx->AreAnalysesValid(IRContext::kAnalysisCFG));
+  EXPECT_TRUE(ctx->AreAnalysesValid(IRContext::kAnalysisStructuredCFG));
+  EXPECT_FALSE(ctx->AreAnalysesValid(IRContext::kAnalysisDominatorAnalysis));
+}
+
+TEST_F(IRContextTest, InvalidateLoopInvalidatesDownstreamOnly) {
+  const std::string text = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+%1 = OpTypeVoid
+%2 = OpTypeFunction %1
+%3 = OpFunction %1 None %2
+%4 = OpLabel
+OpReturn
+OpFunctionEnd)";
+
+  std::unique_ptr<IRContext> ctx =
+      BuildModule(SPV_ENV_UNIVERSAL_1_1, nullptr, text,
+                  SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+
+  ctx->BuildInvalidAnalyses(
+      IRContext::kAnalysisCFG | IRContext::kAnalysisDominatorAnalysis |
+      IRContext::kAnalysisStructuredCFG | IRContext::kAnalysisLoopAnalysis |
+      IRContext::kAnalysisScalarEvolution | IRContext::kAnalysisLiveness);
+
+  ctx->InvalidateAnalyses(IRContext::kAnalysisLoopAnalysis);
+  EXPECT_TRUE(ctx->AreAnalysesValid(IRContext::kAnalysisCFG));
+  EXPECT_TRUE(ctx->AreAnalysesValid(IRContext::kAnalysisDominatorAnalysis));
+  EXPECT_TRUE(ctx->AreAnalysesValid(IRContext::kAnalysisStructuredCFG));
+  EXPECT_FALSE(ctx->AreAnalysesValid(IRContext::kAnalysisLoopAnalysis));
+  EXPECT_FALSE(ctx->AreAnalysesValid(IRContext::kAnalysisScalarEvolution));
+  EXPECT_FALSE(ctx->AreAnalysesValid(IRContext::kAnalysisLiveness));
+}
+
+#ifdef SPIRV_CHECK_CONTEXT
+TEST_F(IRContextTest, IsConsistentCatchesInconsistentDominator) {
+  const std::string text = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+%1 = OpTypeVoid
+%2 = OpTypeFunction %1
+%3 = OpFunction %1 None %2
+%4 = OpLabel
+OpReturn
+OpFunctionEnd)";
+
+  std::unique_ptr<IRContext> ctx =
+      BuildModule(SPV_ENV_UNIVERSAL_1_1, nullptr, text,
+                  SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+
+  ASSERT_TRUE(ctx->module()->begin() != ctx->module()->end());
+  Function* func = &*ctx->module()->begin();
+
+  // Computes and caches dominator analysis
+  auto* dom = ctx->GetDominatorAnalysis(func);
+  EXPECT_TRUE(ctx->IsConsistent());
+
+  // Mess up the cached tree
+  dom->GetDomTree().ClearTree();
+
+  // It should catch the inconsistency
+  EXPECT_FALSE(ctx->IsConsistent());
+}
+
+TEST_F(IRContextTest, IsConsistentCatchesInconsistentStructuredCFG) {
+  const std::string text = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %4 "main"
+               OpExecutionMode %4 OriginUpperLeft
+          %2 = OpTypeVoid
+          %3 = OpTypeFunction %2
+          %11 = OpTypeBool
+          %true = OpConstantTrue %11
+          %4 = OpFunction %2 None %3
+          %5 = OpLabel
+               OpSelectionMerge %10 None
+               OpBranchConditional %true %8 %9
+          %8 = OpLabel
+               OpBranch %10
+          %9 = OpLabel
+               OpBranch %10
+         %10 = OpLabel
+               OpReturn
+               OpFunctionEnd
+  )";
+
+  std::unique_ptr<IRContext> ctx =
+      BuildModule(SPV_ENV_UNIVERSAL_1_3, nullptr, text,
+                  SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+
+  // Build and cache structured CFG analysis
+  ctx->BuildInvalidAnalyses(IRContext::kAnalysisStructuredCFG);
+  EXPECT_TRUE(ctx->IsConsistent());
+
+  // Modify the merge block operand of the selection merge instruction without
+  // invalidating the analysis
+  Instruction* merge_inst = ctx->cfg()->block(5)->GetMergeInst();
+  ASSERT_NE(merge_inst, nullptr);
+  // Change merge block target from %10 to %8 (operand 0 is %10)
+  merge_inst->SetInOperand(0, {8});
+
+  // It should catch the inconsistency
+  EXPECT_FALSE(ctx->IsConsistent());
+}
+#else
+TEST_F(IRContextTest, IsConsistentAlwaysTrueWithoutMacro) {
+  const std::string text = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+%1 = OpTypeVoid
+%2 = OpTypeFunction %1
+%3 = OpFunction %1 None %2
+%4 = OpLabel
+OpReturn
+OpFunctionEnd)";
+
+  std::unique_ptr<IRContext> ctx =
+      BuildModule(SPV_ENV_UNIVERSAL_1_1, nullptr, text,
+                  SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+
+  ASSERT_TRUE(ctx->module()->begin() != ctx->module()->end());
+  Function* func = &*ctx->module()->begin();
+
+  auto* dom = ctx->GetDominatorAnalysis(func);
+  EXPECT_TRUE(ctx->IsConsistent());
+
+  // Mess up the cached tree
+  dom->GetDomTree().ClearTree();
+
+  // Without SPIRV_CHECK_CONTEXT it should still return true
+  EXPECT_TRUE(ctx->IsConsistent());
+}
+#endif
+
 TEST_F(IRContextTest, AsanErrorTest) {
   std::string shader = R"(
                OpCapability Shader
