Repository navigation
Sort cf blocks in program order - #1049
Conversation
How do you prevent DFS from visiting BB4 first? |
|
@Jiahui17 In RPO, a block is added to the postorder only after all its successors have been explored. Regardless of which one is visited first, |
| }]; | ||
| } | ||
|
|
||
| def SortBlocksInProgramOrder : DynamaticPass<"sort-blocks-in-program-order"> { |
There was a problem hiding this comment.
DynamaticPass is deprecated, use Pass instead
"program order" is vague, just use RPO-based CFG order or something similar
|
|
||
| /// Returns the blocks of the region in reverse postorder, starting from its | ||
| /// entry block. Blocks which the entry cannot reach come last. | ||
| static SmallVector<Block *> getBlocksInProgramOrder(Region ®ion) { |
There was a problem hiding this comment.
Using reverse post order instead of program order
|
|
||
| // Moving each block right before the next one, from the last to the first, | ||
| // leaves the region in that same order | ||
| for (size_t idx = order.size(); idx-- > 1;) |
| while (!stack.empty()) { | ||
| Block *block = stack.back().first; | ||
| unsigned nextSuccessor = stack.back().second; | ||
|
|
||
| if (nextSuccessor < block->getNumSuccessors()) { | ||
| stack.back().second = nextSuccessor + 1; | ||
| Block *successor = block->getSuccessor(nextSuccessor); | ||
| if (visited.insert(successor).second) | ||
| stack.push_back({successor, 0}); | ||
| continue; | ||
| } | ||
|
|
||
| // All the successors were explored, thus the block is in postorder position | ||
| programOrder.push_back(block); | ||
| stack.pop_back(); | ||
| } |
There was a problem hiding this comment.
Could you pls describe what's going on here?
| unsigned nextSuccessor = stack.back().second; | ||
|
|
||
| // If there is another successor to explore, remember to continue with the | ||
| // following successor when returning to this block. |
There was a problem hiding this comment.
- "when returning..." When does it return generally?
- "...To this block" is the current block or the nextSuccessor?
Also, why do we do this?
| stack.back().second = nextSuccessor + 1; | ||
| Block *successor = block->getSuccessor(nextSuccessor); | ||
|
|
||
| // Visit the successor if it has not been reached through another path. |
There was a problem hiding this comment.
Again, it is unclear WHY we do this
2 participants
Several parts of the flow assume that basic block IDs reflect program order. In particular,
isBackedgeexplicitly states:However, both
StdProfiler::writeCSVandLowerFuncToHandshake::idBasicBlocksderive block IDs directly from their positions in the region, and nothing currently guarantees this order.This assumption happened to hold for
kmpbefore, but recent frontend changes altered its CFG/block layout and exposed the underlying ordering issue. In the current ordering,bb4 -> bb2andbb9 -> bb7are classified as backedges by their IDs, although neither destination dominates its source. By the standard definition of a backedge (Dragon Book, Sec. 9.6.4), the destination of a backedge must dominate its source.Current ordering, which exposes the issue:
Earlier ordering, where the assumption happened to hold:
This PR adds
--sort-blocks-in-program-order, which explicitly enforces the expected ordering by reordering blocks in reverse postorder (RPO) before their IDs are assigned. RPO gives the ordering expected by the existing ID-based backedge logic for reducible CFGs. Clang similarly uses an RPO-based CFG order and explicitly adjusts it to place loop bodies before loop successors (PostOrderCFGView).The pass runs before profiling and CF-to-Handshake conversion diverge, keeping the block IDs in the frequency CSV and
handshake.bbconsistent.