[SILOptimizer] Clean up infinite recursion diagnostic pass #19710
Conversation
NFC. This cleans up the pass to avoid the extra lambda and to clear up the ordering of what check is supposed to come before what.
| // If we found a recursive call, keep track of it. We can't just `return | ||
| // true` here because that would mark every recursive function infinitely | ||
| // recursive. | ||
| if (hasRecursiveCallInPath(*CurBlock, TargetFn, TargetModule)) { |
There was a problem hiding this comment.
It also defeats the purpose of doing a depth first search!
| SILFunction *TargetFn) { | ||
| /// Perform a DFS through the target function to find any paths to an exit node | ||
| /// that do not call into the target. | ||
| static bool hasInfinitelyRecursiveApply(SILFunction *TargetFn) { |
There was a problem hiding this comment.
N.B. I had the extra parameter here because I intended to extend this pass to compute mutually-recursive applies in the initializer thunks for lazy vars.
| for (SILBasicBlock *Succ : CurBlock->getSuccessorBlocks()) | ||
| foundRecursion |= analyzeSuccessor(Succ); | ||
| // Otherwise, push the successors onto the stack. | ||
| WorkList.append(CurBlock->succblock_begin(), |
There was a problem hiding this comment.
This is where one would do the visited check.
| SmallVector<SILBasicBlock *, 16> WorkList; | ||
| // Keep track of whether we found at least one recursive path. | ||
| bool foundRecursion = false; | ||
| SmallVector<SILBasicBlock *, 16> WorkList = { TargetFn->getEntryBlock() }; |
There was a problem hiding this comment.
Why not do 32 instead of 16? Seems better for medium sized functions and you aren't going to have multiple invocations of this function so the stack usage isn't too crazy to me.
| while (!WorkList.empty()) { | ||
| SILBasicBlock *CurBlock = WorkList.pop_back_val(); | ||
| // Found a path to the exit node without a recursive call. | ||
| if (!Visited.insert(CurBlock).second) |
There was a problem hiding this comment.
It is more efficient to do the check when you know you are going to insert something into the worklist. So you would insert TargetFn->getEntryBlock() as visited and then below when you copy into the worklist, skip anyone that has already been visited.
| // If we found a recursive call, keep track of it. We can't just `return | ||
| // true` here because that would mark every recursive function infinitely | ||
| // recursive. | ||
| if (hasRecursiveCallInPath(*CurBlock, TargetFn, TargetModule)) { |
There was a problem hiding this comment.
Can you change the comment here to make it clear that we are only interested in functions that are recursive along /all/ paths. I think that would get this across better.
|
@swift-ci please smoke test |
|
This is causing failures. See rdar://45080912. Backing out. |
|
Oops, sorry, not this one, the other one (#19724) |
…#19710) * [SILOptimizer] Clean up infinite recursion diagnostic pass NFC. This cleans up the pass to avoid the extra lambda and to clear up the ordering of what check is supposed to come before what. * Address review comments
NFC. This cleans up the pass to avoid the extra lambda and to clear up
the ordering of what check is supposed to come before what. It also
adds a little more documentation about what the pass is checking for.