[clang] [clang][CFG] Change child order in Reverse Post Order (RPO) iteration. (PR #80030)

[Yitzhak Mandelbaum via cfe-commits]

https://github.com/ymand updated https://github.com/llvm/llvm-project/pull/80030

>From 4ea1f14cfd835308064579554c3b8f5ce71ef0c0 Mon Sep 17 00:00:00 2001
From: Yitzhak Mandelbaum <yitzhakm at google.com>
Date: Mon, 29 Jan 2024 21:26:27 +0000
Subject: [PATCH] [clang][CFG] Change child order in Reverse Post Order (RPO)
 iteration.

The CFG orders the blocks of loop bodies before those of loop successors
(both numerically, and in the successor order of the loop condition
block). So, RPO necessarily reverses that order, placing the loop successor
*before* the loop body. For many analyses, particularly those that converge
to a fixpoint, this results in potentially significant extra work, because loop
successors will necessarily need to be reconsidered once the algorithm has
reached a fixpoint on the loop body.

This definition of CFG graph traits reverses the order of children, so that loop
bodies will come first in an RPO. Then, the algorithm can fully evaluate the
loop and only then consider successor blocks.
---
 .../Analysis/Analyses/PostOrderCFGView.h      | 36 ++++++++++++++++++-
 clang/unittests/Analysis/CFGTest.cpp          | 10 +-----
 .../Analysis/FlowSensitive/LoggerTest.cpp     | 15 ++++----
 3 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/PostOrderCFGView.h b/clang/include/clang/Analysis/Analyses/PostOrderCFGView.h
index 4356834adf76f..c4998bb2285f7 100644
--- a/clang/include/clang/Analysis/Analyses/PostOrderCFGView.h
+++ b/clang/include/clang/Analysis/Analyses/PostOrderCFGView.h
@@ -70,7 +70,41 @@ class PostOrderCFGView : public ManagedAnalysis {
   };
 
 private:
-  using po_iterator = llvm::po_iterator<const CFG *, CFGBlockSet, true>;
+  // The CFG orders the blocks of loop bodies before those of loop successors
+  // (both numerically, and in the successor order of the loop condition
+  // block). So, RPO necessarily reverses that order, placing the loop successor
+  // *before* the loop body. For many analyses, particularly those that converge
+  // to a fixpoint, this results in potentially significant extra work because
+  // loop successors will necessarily need to be reconsidered once the algorithm
+  // has reached a fixpoint on the loop body.
+  //
+  // This definition of CFG graph traits reverses the order of children, so that
+  // loop bodies will come first in an RPO.
+  struct CFGLoopBodyFirstTraits {
+    using NodeRef = const ::clang::CFGBlock *;
+    using ChildIteratorType = ::clang::CFGBlock::const_succ_reverse_iterator;
+
+    static ChildIteratorType child_begin(NodeRef N) { return N->succ_rbegin(); }
+    static ChildIteratorType child_end(NodeRef N) { return N->succ_rend(); }
+
+    using nodes_iterator = ::clang::CFG::const_iterator;
+
+    static NodeRef getEntryNode(const ::clang::CFG *F) {
+      return &F->getEntry();
+    }
+
+    static nodes_iterator nodes_begin(const ::clang::CFG *F) {
+      return F->nodes_begin();
+    }
+
+    static nodes_iterator nodes_end(const ::clang::CFG *F) {
+      return F->nodes_end();
+    }
+
+    static unsigned size(const ::clang::CFG *F) { return F->size(); }
+  };
+  using po_iterator =
+      llvm::po_iterator<const CFG *, CFGBlockSet, true, CFGLoopBodyFirstTraits>;
   std::vector<const CFGBlock *> Blocks;
 
   using BlockOrderTy = llvm::DenseMap<const CFGBlock *, unsigned>;
diff --git a/clang/unittests/Analysis/CFGTest.cpp b/clang/unittests/Analysis/CFGTest.cpp
index 9fa034a0e472e..2b27da0081425 100644
--- a/clang/unittests/Analysis/CFGTest.cpp
+++ b/clang/unittests/Analysis/CFGTest.cpp
@@ -276,14 +276,6 @@ TEST(CFG, Worklists) {
                            ForwardNodes.begin()));
   }
 
-  // RPO: 876321054
-  // WTO: 876534210
-  // So, we construct the WTO order accordingly from the reference order.
-  std::vector<const CFGBlock *> WTOOrder = {
-      ReferenceOrder[0], ReferenceOrder[1], ReferenceOrder[2],
-      ReferenceOrder[7], ReferenceOrder[3], ReferenceOrder[8],
-      ReferenceOrder[4], ReferenceOrder[5], ReferenceOrder[6]};
-
   {
     using ::testing::ElementsAreArray;
     std::optional<WeakTopologicalOrdering> WTO = getIntervalWTO(*CFG);
@@ -297,7 +289,7 @@ TEST(CFG, Worklists) {
     while (const CFGBlock *B = WTOWorklist.dequeue())
       WTONodes.push_back(B);
 
-    EXPECT_THAT(WTONodes, ElementsAreArray(WTOOrder));
+    EXPECT_THAT(WTONodes, ElementsAreArray(*WTO));
   }
 
   std::reverse(ReferenceOrder.begin(), ReferenceOrder.end());
diff --git a/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp b/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
index c5594aa3fe9d1..57920c49a7d3d 100644
--- a/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
@@ -5,7 +5,6 @@
 #include "clang/Analysis/FlowSensitive/DataflowLattice.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
-#include <optional>
 
 namespace clang::dataflow::test {
 namespace {
@@ -90,7 +89,7 @@ class TestLogger : public Logger {
 AnalysisInputs<TestAnalysis> makeInputs() {
   const char *Code = R"cpp(
 int target(bool b, int p, int q) {
-  return b ? p : q;    
+  return b ? p : q;
 }
 )cpp";
   static const std::vector<std::string> Args = {
@@ -125,17 +124,17 @@ transfer()
 recordState(Elements=2, Branches=0, Joins=0)
 recordState(Elements=2, Branches=0, Joins=0)
 
-enterBlock(3, false)
-transferBranch(0)
+enterBlock(2, false)
+transferBranch(1)
 recordState(Elements=2, Branches=1, Joins=0)
-enterElement(q)
+enterElement(p)
 transfer()
 recordState(Elements=3, Branches=1, Joins=0)
 
-enterBlock(2, false)
-transferBranch(1)
+enterBlock(3, false)
+transferBranch(0)
 recordState(Elements=2, Branches=1, Joins=0)
-enterElement(p)
+enterElement(q)
 transfer()
 recordState(Elements=3, Branches=1, Joins=0)