JIT: Merge all RETURN/THROW blocks#128515
Conversation
github-actions
Bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
Bot
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@AndyAyersMS PTAL. |
| // Avoid splitting a return away from a possible tail call | ||
| // | ||
| if (!block->hasSingleStmt()) | ||
| if (block->isEmpty()) |
There was a problem hiding this comment.
This check was here before, but I dont think we actually need it. Because we only accept RETURN or THROW blocks and these should never be empty?
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
Can we do this without repeatedly searching all blocks for returns and throws?
| do | ||
| { | ||
| predInfo.Reset(); | ||
| for (BasicBlock* const block : Blocks()) |
There was a problem hiding this comment.
The set of eligible return and throw blocks never changes, so do we need to repeatedly walk the entire block list here?
There was a problem hiding this comment.
I would also think we don't need to, however when I tried to hoist that it caused asserts.
I didn't look further into it because the same approach is already done in iterateTailMerge() and there are also multiple comments arround this code about improving algorithm efficiency.
So I'd prefer properly understanding the entire code and improving efficiency in a separate PR, in the future.
There was a problem hiding this comment.
iterateTailMerge just walks the preds of a given block, not all blocks.
What asserts did you see?
There was a problem hiding this comment.
iterateTailMerge just walks the preds of a given block, not all blocks.
Yeah it happens to be only the preds here and so less of an issue but the fundamental thing of not needing to regenerate the set still applies I think.
What asserts did you see?
runtime/src/coreclr/jit/fgstmt.cpp
Lines 542 to 550 in 7f58900
Took a quick look, the issue might be that we don't remove entries from predInfo after we merged them.
So it will try to merge them a second time on the second iter - and is never making any progress.
Let me see if I can fix it...
2 participants
Fix #128514
tailMergePreds(nullptr)was called once, but my understanding is it needs to be called repeatedly as it only processes one set at at time.