Open
Conversation
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
* simplify IfConvertCheckStmts * small things
9792586 to
5c1616c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR simplifies the CoreCLR JIT if-conversion implementation by collapsing the prior multi-block flow search into a single, direct-flow check, under the assumption that Then/Else paths do not contain empty/chained blocks.
Changes:
- Replace the prior bounded “walk” of Then/Else chains with a single
IfConvertCheckFlow()check. - Simplify statement validation to operate on a single block (ignoring NOPs) rather than scanning a block chain to
m_finalBlock. - Simplify debug dumping and block removal to target only the identified Then/Else blocks.
Comment on lines
+83
to
+100
| m_doElseConversion = trueBb->GetUniquePred(m_compiler) != nullptr; | ||
| m_finalBlock = m_doElseConversion ? trueBb->GetUniqueSucc() : trueBb; | ||
| m_mainOper = GT_STORE_LCL_VAR; | ||
|
|
||
| for (int thenLimit = 0; thenLimit < m_checkLimit; thenLimit++) | ||
| if (m_finalBlock == nullptr) | ||
| { | ||
| if (!IfConvertCheckInnerBlockFlow(thenBlock)) | ||
| assert(m_doElseConversion); | ||
| if (falseBb->KindIs(BBJ_RETURN) && trueBb->KindIs(BBJ_RETURN)) | ||
| { | ||
| // Then block is not in a valid flow. | ||
| return true; | ||
| m_mainOper = GT_RETURN; | ||
| } | ||
| BasicBlock* thenBlockNext = thenBlock->GetUniqueSucc(); | ||
|
|
||
| if (thenBlockNext == m_finalBlock) | ||
| { | ||
| // All the Then blocks up to m_finalBlock are in a valid flow. | ||
| m_flowFound = true; | ||
| if (thenBlock->KindIs(BBJ_RETURN)) | ||
| { | ||
| assert(m_finalBlock == nullptr); | ||
| m_mainOper = GT_RETURN; | ||
| } | ||
| else | ||
| { | ||
| m_mainOper = GT_STORE_LCL_VAR; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| if (thenBlockNext == nullptr) | ||
| else | ||
| { | ||
| // Invalid Then and Else combination. | ||
| return false; | ||
| } | ||
|
|
||
| thenBlock = thenBlockNext; | ||
| } | ||
|
|
||
| // Nothing found. Still valid to continue. | ||
| return true; | ||
| } | ||
|
|
||
| //----------------------------------------------------------------------------- | ||
| // IfConvertFindFlow | ||
| // | ||
| // Find a valid if conversion flow from m_startBlock to a final block. | ||
| // There might be multiple Then and Else blocks in the flow - use m_checkLimit to limit this. | ||
| // | ||
| // Notes: | ||
| // Sets m_flowFound, m_finalBlock, m_doElseConversion and m_mainOper. | ||
| // | ||
| void OptIfConversionDsc::IfConvertFindFlow() | ||
| { | ||
| // First check for flow with no else case. The final block is the destination of the jump. | ||
| m_doElseConversion = false; | ||
| m_finalBlock = m_startBlock->GetTrueTarget(); | ||
| assert(m_finalBlock != nullptr); | ||
| if (!IfConvertCheckThenFlow() || m_flowFound) | ||
| { | ||
| // Either the flow is invalid, or a flow was found. | ||
| return; | ||
| } | ||
|
|
||
| // Look for flows with else blocks. The final block is the block after the else block. | ||
| m_doElseConversion = true; | ||
| for (int elseLimit = 0; elseLimit < m_checkLimit; elseLimit++) | ||
| { | ||
| BasicBlock* elseBlock = m_finalBlock; | ||
| if (elseBlock == nullptr || !IfConvertCheckInnerBlockFlow(elseBlock)) | ||
| { | ||
| // Need a valid else block in a valid flow . | ||
| return; | ||
| } | ||
|
|
||
| m_finalBlock = elseBlock->GetUniqueSucc(); | ||
|
|
||
| if (!IfConvertCheckThenFlow() || m_flowFound) | ||
| { | ||
| // Either the flow is invalid, or a flow was found. | ||
| return; | ||
| } | ||
| } | ||
| return falseBb->GetUniqueSucc() == m_finalBlock; |
There was a problem hiding this comment.
IfConvertCheckFlow now requires the Then/Else blocks to be direct predecessors of the merge (via GetUniqueSucc equality). However the JIT can introduce empty BBJ_ALWAYS fallthrough blocks (e.g., BBF_INTERNAL blocks from fgNormalizeEH), which previously would have been handled by walking a short chain. This makes the change unlikely to be truly “zero-diff” and can silently disable if-conversion for methods with EH normalization artifacts. Consider skipping/peeling empty fallthrough blocks (similar to SkipFallthroughBlocks in switchrecognition.cpp) or reintroducing a bounded walk so the optimization remains robust in the presence of empty blocks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Zero-diff change that simplifies if-conversion based on the assumption that there are no empty blocks inside the Then and Else case. See: #125072 (comment)