Conversation
andmadri
force-pushed
the
type-optimizer-changes
branch
from
90981f4 to
a97fa00
Compare
yuri91
left a comment
yuri91
left a comment
There was a problem hiding this comment.
Looks good to me.
I would appreciate a second look from @alexp-sssup to a2bf1af specifically.
It's the only actual change to the algorithm.
|
@yuri91 @andmadri I think the logic is not aggressive enough, specifically there is no point in iterating for a few times to decide the improvements have stalled. If the logic is sound it should be equivalent to stop immediately. Could we validate if there is ever any case where progress is made in a second iteration after the first has not made any? |
andmadri
force-pushed
the
type-optimizer-changes
branch
3 times, most recently
from
b80cfd9 to
2ac6f3f
Compare
| }; | ||
| llvm::Function& F; | ||
| std::vector<std::pair<llvm::BasicBlock*, llvm::BasicBlock*>> existingEdges; | ||
|
|
There was a problem hiding this comment.
Nit, avoid spurious changes
| for (llvm::BasicBlock* succ : visitNext) | ||
| registerEdge(start, succ); | ||
|
|
||
| if (parentNode) |
There was a problem hiding this comment.
I find this logic somewhat confusing, although I don't recall exactly how recursion works here. Two things.
-
I think the std::stack object here is redundant, you could use just a flag per SCC. Moreover, do we even need to store it in parentNode? Could we return it from each children in recursiveVisit?
-
Doing the check here is counterintuitive, it should be hoisted and done in the context of the parent rather than doing it in the context of the children using parentNode.
@yuri91 for further feedback
There was a problem hiding this comment.
A flag could work but I don't think we can return it from each children in recursiveVisit because of how the recursion works. The issue is that when we reach the last child and it loops back to itself, the recursion doesn’t unwind to the parent. Instead, we recurse again from that child, as that child has the same SCC set. That is why I am checking the flag/stack at that point. Either we decide to continue recursing through that set of the same SCCs until we hit the iteration limit or we resolve it, or we return to the parent and move on.
andmadri
force-pushed
the
type-optimizer-changes
branch
5 times, most recently
from
f89fb50 to
e0cfe14
Compare
The parent tracks whether progress has been made while processing its SCC set. Each visit to a child SCC returns a boolean indicating whether running that child resulted in progress. Children are visited in reverse order. The last child is always the starting SCC. The first child is a copy that contains all basic blocks split into SCCs and used to detect loops. This copy is marked reachable when, during edge registration, we detect a back-edge into the current SCC set. If the copy is reachable, the SCC set forms a loop. After all SCCs have ben analyzed, we use the progress flag to decide whether to visit the copy again. If progress was made, the copy is visited, which recreates the same SCC structure with a new copy at the end. If no progress was made, we stop recursing that set of SCC and mark them as skipped.
andmadri
force-pushed
the
type-optimizer-changes
branch
from
e0cfe14 to
fed9cf3
Compare
3 participants
Most of the optimizations are in recursiveVisit() and splitIntoSCCs(). Optimized splitIntoSCCs when blocks size was one, there was no need to split into SCC as that is pretty costly. I aslo find a way to exit earlier rather than alwasy looping a 100 times until we decide to give up on a SCC.