From ae0a74e82b3e4e4469333bac011fba5fdce6efa1 Mon Sep 17 00:00:00 2001 From: Jesse Natalie Date: Thu, 5 Mar 2026 13:29:59 -0800 Subject: [PATCH 1/3] Change scope nested CFG passes to use a stack instead of recursion --- .../lib/DxilConvPasses/ScopeNestedCFG.cpp | 301 ++++++++++-------- 1 file changed, 174 insertions(+), 127 deletions(-) diff --git a/projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp b/projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp index 3744568e53..e27fad6933 100644 --- a/projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp +++ b/projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp @@ -14,6 +14,7 @@ #include "llvm/Analysis/ReducibilityAnalysis.h" #include "llvm/ADT/BitVector.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/Analysis/CFG.h" #include "llvm/Analysis/LoopPass.h" #include "llvm/IR/CFG.h" @@ -140,8 +141,6 @@ class ScopeNestedCFG : public FunctionPass { // Preliminary CFG transformations and related utilities. // void SanitizeBranches(); - void SanitizeBranchesRec(BasicBlock *pBB, - unordered_set &VisitedBB); void CollectUniqueSuccessors(const BasicBlock *pBB, const BasicBlock *pSuccessorToExclude, vector &Successors); @@ -149,7 +148,7 @@ class ScopeNestedCFG : public FunctionPass { // // Loop region transformations. // - void CollectLoopsRec(Loop *pLoop); + void CollectLoops(Loop *pLoop); void AnnotateBranch(BasicBlock *pBB, BranchKind Kind); BranchKind GetBranchAnnotation(const BasicBlock *pBB); @@ -203,9 +202,6 @@ class ScopeNestedCFG : public FunctionPass { }; void ComputeBlockTopologicalOrderAndReachability( BasicBlock *pEntry, BlockTopologicalOrderAndReachability &BTO); - void ComputeBlockTopologicalOrderAndReachabilityRec( - BasicBlock *pNode, BlockTopologicalOrderAndReachability &BTO, - unordered_map &Marks); // // Recovery of scope end points. @@ -337,7 +333,7 @@ bool ScopeNestedCFG::runOnFunction(Function &F) { for (auto itLoop = m_pLI->begin(), endLoop = m_pLI->end(); itLoop != endLoop; ++itLoop) { Loop *pLoop = *itLoop; - CollectLoopsRec(pLoop); + CollectLoops(pLoop); } // @@ -428,91 +424,90 @@ void ScopeNestedCFG::Clear() { // Preliminary CFG transformations and related utilities. //----------------------------------------------------------------------------- void ScopeNestedCFG::SanitizeBranches() { - unordered_set VisitedBB; - SanitizeBranchesRec(m_pFunc->begin(), VisitedBB); -} + SmallPtrSet VisitedBB; + SmallVector Worklist; + Worklist.push_back(m_pFunc->begin()); -void ScopeNestedCFG::SanitizeBranchesRec( - BasicBlock *pBB, unordered_set &VisitedBB) { - // Mark pBB as visited, and return if pBB already has been visited. - if (!VisitedBB.emplace(pBB).second) - return; + while (!Worklist.empty()) { + BasicBlock *pBB = Worklist.pop_back_val(); - // Sanitize branch. - if (BranchInst *I = dyn_cast(pBB->getTerminator())) { - // a. Convert a conditional branch to unconditional, if successors are the - // same. - if (I->isConditional()) { - BasicBlock *pSucc1 = I->getSuccessor(0); - BasicBlock *pSucc2 = I->getSuccessor(1); - if (pSucc1 == pSucc2) { - BranchInst::Create(pSucc1, I); - I->eraseFromParent(); + // Mark pBB as visited, and skip if pBB already has been visited. + if (!VisitedBB.insert(pBB).second) + continue; + + // Sanitize branch. + if (BranchInst *I = dyn_cast(pBB->getTerminator())) { + // a. Convert a conditional branch to unconditional, if successors are the + // same. + if (I->isConditional()) { + BasicBlock *pSucc1 = I->getSuccessor(0); + BasicBlock *pSucc2 = I->getSuccessor(1); + if (pSucc1 == pSucc2) { + BranchInst::Create(pSucc1, I); + I->eraseFromParent(); + } } - } - } else if (SwitchInst *I = dyn_cast(pBB->getTerminator())) { - // b. Group switch successors. - struct SwitchCaseGroup { - BasicBlock *pSuccBB; - vector CaseValue; - }; - vector SwitchCaseGroups; - unordered_map BB2GroupIdMap; - BasicBlock *pDefaultBB = I->getDefaultDest(); + } else if (SwitchInst *I = dyn_cast(pBB->getTerminator())) { + // b. Group switch successors. + struct SwitchCaseGroup { + BasicBlock *pSuccBB; + SmallVector CaseValue; + }; + SmallVector SwitchCaseGroups; + DenseMap BB2GroupIdMap; + BasicBlock *pDefaultBB = I->getDefaultDest(); - for (SwitchInst::CaseIt itCase = I->case_begin(), endCase = I->case_end(); - itCase != endCase; ++itCase) { - BasicBlock *pSuccBB = itCase.getCaseSuccessor(); - ConstantInt *pCaseValue = itCase.getCaseValue(); + for (SwitchInst::CaseIt itCase = I->case_begin(), endCase = I->case_end(); + itCase != endCase; ++itCase) { + BasicBlock *pSuccBB = itCase.getCaseSuccessor(); + ConstantInt *pCaseValue = itCase.getCaseValue(); - if (pSuccBB == pDefaultBB) { // Assimilate this case label into default label. - continue; - } + if (pSuccBB == pDefaultBB) + continue; - auto itGroup = BB2GroupIdMap.insert({pSuccBB, SwitchCaseGroups.size()}); - if (itGroup.second) { - SwitchCaseGroups.emplace_back(SwitchCaseGroup{}); - } + auto itGroup = BB2GroupIdMap.insert({pSuccBB, SwitchCaseGroups.size()}); + if (itGroup.second) + SwitchCaseGroups.emplace_back(SwitchCaseGroup{}); - SwitchCaseGroup &G = SwitchCaseGroups[itGroup.first->second]; - G.pSuccBB = pSuccBB; - G.CaseValue.emplace_back(pCaseValue); - } + SwitchCaseGroup &G = SwitchCaseGroups[itGroup.first->second]; + G.pSuccBB = pSuccBB; + G.CaseValue.emplace_back(pCaseValue); + } - if (SwitchCaseGroups.size() == 0) { - // All case labels were assimilated into the default label. - // Replace switch with an unconditional branch. - BranchInst::Create(pDefaultBB, I); - I->eraseFromParent(); - } else { - // Rewrite switch instruction such that case labels are grouped by the - // successor. - unsigned CaseIdx = 0; - for (const SwitchCaseGroup &G : SwitchCaseGroups) { - for (ConstantInt *pCaseValue : G.CaseValue) { - SwitchInst::CaseIt itCase(I, CaseIdx++); - itCase.setSuccessor(G.pSuccBB); - itCase.setValue(pCaseValue); + if (SwitchCaseGroups.size() == 0) { + // All case labels were assimilated into the default label. + // Replace switch with an unconditional branch. + BranchInst::Create(pDefaultBB, I); + I->eraseFromParent(); + } else { + // Rewrite switch instruction such that case labels are grouped by the + // successor. + unsigned CaseIdx = 0; + for (const SwitchCaseGroup &G : SwitchCaseGroups) { + for (ConstantInt *pCaseValue : G.CaseValue) { + SwitchInst::CaseIt itCase(I, CaseIdx++); + itCase.setSuccessor(G.pSuccBB); + itCase.setValue(pCaseValue); + } + } + // Remove unused case labels. + for (unsigned NumCases = I->getNumCases(); CaseIdx < NumCases; + NumCases--) { + I->removeCase(SwitchInst::CaseIt{I, NumCases - 1}); } - } - // Remove unused case labels. - for (unsigned NumCases = I->getNumCases(); CaseIdx < NumCases; - NumCases--) { - I->removeCase(SwitchInst::CaseIt{I, NumCases - 1}); } } - } - // Recurse, visiting each successor group once. - TerminatorInst *pTI = pBB->getTerminator(); - BasicBlock *pPrevSuccBB = nullptr; - for (unsigned i = 0; i < pTI->getNumSuccessors(); i++) { - BasicBlock *pSuccBB = pTI->getSuccessor(i); - if (pSuccBB != pPrevSuccBB) { - SanitizeBranchesRec(pSuccBB, VisitedBB); + // Push successors onto worklist, visiting each successor group once. + TerminatorInst *pTI = pBB->getTerminator(); + BasicBlock *pPrevSuccBB = nullptr; + for (unsigned i = 0; i < pTI->getNumSuccessors(); i++) { + BasicBlock *pSuccBB = pTI->getSuccessor(i); + if (pSuccBB != pPrevSuccBB) + Worklist.push_back(pSuccBB); + pPrevSuccBB = pSuccBB; } - pPrevSuccBB = pSuccBB; } } @@ -537,14 +532,24 @@ void ScopeNestedCFG::CollectUniqueSuccessors( //----------------------------------------------------------------------------- // Loop region transformations. //----------------------------------------------------------------------------- -void ScopeNestedCFG::CollectLoopsRec(Loop *pLoop) { - for (auto itLoop = pLoop->begin(), endLoop = pLoop->end(); itLoop != endLoop; - ++itLoop) { - Loop *pNestedLoop = *itLoop; - CollectLoopsRec(pNestedLoop); +void ScopeNestedCFG::CollectLoops(Loop *pLoop) { + // Iterative post-order: collect in reverse pre-order, then reverse. + SmallVector Stack; + SmallVector ReversePostOrder; + Stack.push_back(pLoop); + + while (!Stack.empty()) { + Loop *pCurrent = Stack.pop_back_val(); + ReversePostOrder.push_back(pCurrent); + for (auto it = pCurrent->begin(), end = pCurrent->end(); it != end; ++it) { + Stack.push_back(*it); + } } - m_Loops.emplace_back(pLoop); + for (auto it = ReversePostOrder.rbegin(), end = ReversePostOrder.rend(); + it != end; ++it) { + m_Loops.emplace_back(*it); + } } void ScopeNestedCFG::AnnotateBranch(BasicBlock *pBB, BranchKind Kind) { @@ -988,60 +993,102 @@ void ScopeNestedCFG::BlockTopologicalOrderAndReachability::dump( void ScopeNestedCFG::ComputeBlockTopologicalOrderAndReachability( BasicBlock *pEntry, BlockTopologicalOrderAndReachability &BTO) { - unordered_map WaterMarks; - ComputeBlockTopologicalOrderAndReachabilityRec(pEntry, BTO, WaterMarks); + DenseMap Marks; + unsigned NumBBs = (unsigned)pEntry->getParent()->getBasicBlockList().size(); + + struct StackFrame { + BasicBlock *pNode; + BasicBlock *pEffective; + unique_ptr ReachableBBs; + succ_iterator CurrentSucc; + succ_iterator EndSucc; + bool Initialized; + + StackFrame(BasicBlock *Node) + : pNode(Node), pEffective(nullptr), CurrentSucc(succ_begin(Node)), + EndSucc(succ_end(Node)), Initialized(false) {} + }; -#if SNCFG_DBG - dbgs() << "\nBB topological order and reachable BBs rooted at " - << pEntry->getName() << ":\n"; - BTO.dump(dbgs()); -#endif -} + SmallVector Stack; + Stack.emplace_back(pEntry); -void ScopeNestedCFG::ComputeBlockTopologicalOrderAndReachabilityRec( - BasicBlock *pNode, BlockTopologicalOrderAndReachability &BTO, - unordered_map &Marks) { - auto itMarkBB = Marks.find(pNode); - if (Marks.find(pNode) != Marks.end()) { - DXASSERT(itMarkBB->second == 2, "acyclic component has a cycle"); - return; - } + while (!Stack.empty()) { + StackFrame &Frame = Stack.back(); - unsigned NumBBs = (unsigned)pNode->getParent()->getBasicBlockList().size(); + if (!Frame.Initialized) { + auto itMark = Marks.find(Frame.pNode); + if (itMark != Marks.end()) { + DXASSERT(itMark->second == 2, "acyclic component has a cycle"); + Stack.pop_back(); + continue; + } - // Region terminator. - if (IsAcyclicRegionTerminator(pNode)) { - Marks[pNode] = 2; // late watermark - BTO.AppendBlock(pNode, std::make_unique(NumBBs, false)); - return; - } + // Region terminator. + if (IsAcyclicRegionTerminator(Frame.pNode)) { + Marks[Frame.pNode] = 2; // late watermark + BTO.AppendBlock(Frame.pNode, + std::make_unique(NumBBs, false)); + Stack.pop_back(); + continue; + } - BasicBlock *pNodeToFollowSuccessors = - GetEffectiveNodeToFollowSuccessor(pNode); + BasicBlock *pEffective = GetEffectiveNodeToFollowSuccessor(Frame.pNode); - // Loop with no exit. - if (pNodeToFollowSuccessors == nullptr) { - Marks[pNode] = 2; // late watermark - BTO.AppendBlock(pNode, std::make_unique(NumBBs, false)); - return; - } + // Loop with no exit. + if (pEffective == nullptr) { + Marks[Frame.pNode] = 2; // late watermark + BTO.AppendBlock(Frame.pNode, + std::make_unique(NumBBs, false)); + Stack.pop_back(); + continue; + } - Marks[pNode] = 1; // early watermark + Frame.Initialized = true; + Frame.pEffective = pEffective; + Marks[Frame.pNode] = 1; // early watermark + Frame.ReachableBBs = std::make_unique(NumBBs, false); + Frame.CurrentSucc = succ_begin(pEffective); + Frame.EndSucc = succ_end(pEffective); + } - auto ReachableBBs = std::make_unique(NumBBs, false); - for (auto itSucc = succ_begin(pNodeToFollowSuccessors), - endSucc = succ_end(pNodeToFollowSuccessors); - itSucc != endSucc; ++itSucc) { - BasicBlock *pSuccBB = *itSucc; + // Process successors one at a time. When an unvisited successor is found, + // push it and revisit this frame later (without advancing the iterator so + // the now-visited successor will be unioned on the next visit). + bool PushedChild = false; + while (Frame.CurrentSucc != Frame.EndSucc) { + BasicBlock *pSuccBB = *Frame.CurrentSucc; + auto itMark = Marks.find(pSuccBB); + if (itMark != Marks.end()) { + DXASSERT(itMark->second == 2, "acyclic component has a cycle"); + // Already processed, union reachable BBs and advance. + (*Frame.ReachableBBs) |= (*BTO.GetReachableBBs(pSuccBB)); + ++Frame.CurrentSucc; + } else { + // Successor not yet visited; push it for processing. + // Don't advance the iterator: when we return here the successor + // will be marked and we will union its reachability above. + Stack.emplace_back(pSuccBB); + PushedChild = true; + break; + } + } - ComputeBlockTopologicalOrderAndReachabilityRec(pSuccBB, BTO, Marks); - // Union reachable BBs. - (*ReachableBBs) |= (*BTO.GetReachableBBs(pSuccBB)); - } + if (PushedChild) + continue; - Marks[pNode] = 2; // late watermark + // All successors processed. + BasicBlock *pNode = Frame.pNode; + auto ReachableBBs = std::move(Frame.ReachableBBs); + Marks[pNode] = 2; // late watermark + BTO.AppendBlock(pNode, std::move(ReachableBBs)); + Stack.pop_back(); + } - BTO.AppendBlock(pNode, std::move(ReachableBBs)); +#if SNCFG_DBG + dbgs() << "\nBB topological order and reachable BBs rooted at " + << pEntry->getName() << ":\n"; + BTO.dump(dbgs()); +#endif } //----------------------------------------------------------------------------- From 0e8f40b4290d0bbc91b8c0836eac76e38d6c8007 Mon Sep 17 00:00:00 2001 From: Jesse Natalie Date: Thu, 5 Mar 2026 14:01:23 -0800 Subject: [PATCH 2/3] Apply self-review suggestions --- projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp b/projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp index e27fad6933..9cdbf040b0 100644 --- a/projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp +++ b/projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp @@ -493,9 +493,8 @@ void ScopeNestedCFG::SanitizeBranches() { } // Remove unused case labels. for (unsigned NumCases = I->getNumCases(); CaseIdx < NumCases; - NumCases--) { + NumCases--) I->removeCase(SwitchInst::CaseIt{I, NumCases - 1}); - } } } @@ -541,15 +540,13 @@ void ScopeNestedCFG::CollectLoops(Loop *pLoop) { while (!Stack.empty()) { Loop *pCurrent = Stack.pop_back_val(); ReversePostOrder.push_back(pCurrent); - for (auto it = pCurrent->begin(), end = pCurrent->end(); it != end; ++it) { + for (auto it = pCurrent->begin(), end = pCurrent->end(); it != end; ++it) Stack.push_back(*it); - } } for (auto it = ReversePostOrder.rbegin(), end = ReversePostOrder.rend(); - it != end; ++it) { + it != end; ++it) m_Loops.emplace_back(*it); - } } void ScopeNestedCFG::AnnotateBranch(BasicBlock *pBB, BranchKind Kind) { From 7d2417ee092987f9dc327291f38369a30a5e13d8 Mon Sep 17 00:00:00 2001 From: Jesse Natalie Date: Thu, 5 Mar 2026 15:12:02 -0800 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Chris B --- projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp b/projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp index 9cdbf040b0..3e8bc26960 100644 --- a/projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp +++ b/projects/dxilconv/lib/DxilConvPasses/ScopeNestedCFG.cpp @@ -493,7 +493,7 @@ void ScopeNestedCFG::SanitizeBranches() { } // Remove unused case labels. for (unsigned NumCases = I->getNumCases(); CaseIdx < NumCases; - NumCases--) + --NumCases) I->removeCase(SwitchInst::CaseIt{I, NumCases - 1}); } } @@ -1075,7 +1075,7 @@ void ScopeNestedCFG::ComputeBlockTopologicalOrderAndReachability( // All successors processed. BasicBlock *pNode = Frame.pNode; - auto ReachableBBs = std::move(Frame.ReachableBBs); + unique_ptr ReachableBBs = std::move(Frame.ReachableBBs); Marks[pNode] = 2; // late watermark BTO.AppendBlock(pNode, std::move(ReachableBBs)); Stack.pop_back();