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

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.

Author: ymand

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>;

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());

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)