Change scope nested CFG passes to use a stack instead of recursion#8227
Change scope nested CFG passes to use a stack instead of recursion#8227jenatali merged 3 commits intomicrosoft:mainfrom
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
llvm-beanz
left a comment
There was a problem hiding this comment.
This does not conform to our coding standards some obvious violations include:
b2475f9 to
ae0a74e
Compare
llvm-beanz
left a comment
There was a problem hiding this comment.
Thank you for the cleanups! I just have two small nits, otherwise this looks good to me.
Co-authored-by: Chris B <beanz@abolishcrlf.org>
| (*ReachableBBs) |= (*BTO.GetReachableBBs(pSuccBB)); | ||
| } | ||
| if (PushedChild) | ||
| continue; |
There was a problem hiding this comment.
I can't wrap my mind around how this is logically equivalent to the previous code, this looks like a behavioral change. Lines 1054-1074 here are the only lines I can't see the equivalent iterative translation for. Can you explain why we continue here? Maybe that would help explain the previous lines to me also. Other than those lines, this change looks good to me.
There was a problem hiding this comment.
The previous logic would:
- Check for work to do for the target block
- For each successor:
- Recurse
- After recursion for a specific block, update the reachable blocks
- After all recursion, update the late watermark and append to BTO
The new logic will:
- In an iteration, check if this is the "before recursion" phase (based on the initialized flag), and if so, do the pre-recursion work.
- Process one successor: For the current successor, if we haven't processed it, then push a stack frame, break out of the successor loop, and immediately proceed back to the top of the iteration loop (i.e. begin processing that new frame). If we've already processed the successor, then iterate until we find one we haven't processed, or until we've checked all of them.
- If we didn't push a recursive stack frame, then process the post-recursion work.
bob80905
left a comment
There was a problem hiding this comment.
The continue makes sense now. There is an extra DXAssert that is being introduced that I don't think was in the previous version, and it might be redundant since this assert already exists earlier in the function on what appears to be an already-processed frame. But otherwise LGTM.
Fixes #8224