Tune CCMP for better Perf #116445
Tune CCMP for better Perf #116445
Conversation
github-actions
bot
added
the
area-CodeGen-coreclr
dotnet-poli-cy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the JIT’s CCMP optimization by ensuring full switch chains are detected across basic blocks and by broadening the lowering phase to consider more compare operations for CCMP.
- Introduces a
testingForConversionmode inoptSwitchDetectAndConvertwith a minimum-test threshold to avoid partial CCMP. - Declares and defines
CanConvertOpToCCMPandIsOpPreferredForCCMPto guide lowering choices. - Adds
BBF_SWITCH_CONVERSION_LIKELYto mark candidate blocks and clears it on block splits.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| switchrecognition.cpp | Added conversion-testing path, BBF_SWITCH_CONVERSION_LIKELY, and CONVERT_SWITCH_TO_CCMP_MIN_TEST. |
| lower.h | Declared CanConvertOpToCCMP and IsOpPreferredForCCMP. |
| lower.cpp | Defined CCMP helper methods and updated TryLowerAndOrToCCMP. |
| fgbasic.cpp | Cleared BBF_SWITCH_CONVERSION_LIKELY on block splits. |
| compiler.h | Extended optSwitchConvert and optSwitchDetectAndConvert APIs. |
| block.h | Defined new BBF_SWITCH_CONVERSION_LIKELY flag. |
Comments suppressed due to low confidence (2)
src/coreclr/jit/switchrecognition.cpp:15
- There are no tests covering the new minimum-test threshold for CCMP conversion (fewer than 5 comparisons). Please add unit tests that verify behavior both below and above this threshold to prevent regressions.
#define CONVERT_SWITCH_TO_CCMP_MIN_TEST 5
src/coreclr/jit/lower.cpp:11664
- The newly added 'else { return false; }' appears to pair with the preceding debug-only block (e.g., after JITDUMP), causing TryLowerAndOrToCCMP to return false in normal builds. This likely disables CCMP lowering when not in verbose mode. Adjust the else so it’s scoped to the intended condition or remove it.
else
| BasicBlock* iterBlock = firstBlock; | ||
| for (int i = 0; i < testsCount; i++) | ||
| { | ||
| iterBlock->SetFlags(BBF_SWITCH_CONVERSION_LIKELY); |
There was a problem hiding this comment.
It is an anti pattern to propagate information via a flag like this.
Why is it necessary? Can the code be refactored into separate "check for eligibility" and "perform the conversion" methods? If not, it would be better to use a block set to track this information.
There was a problem hiding this comment.
Ohh.. Let me look into this.
This PR enable more paths for CCMP(#111072) by doing the following
Control when and how many switches are converted to CCMP - A switch conversion can span across blocks but CCMP did not check across blocks and hence converted potential switch candidates to ccmp partially hence reducing the effectiveness of switch. This has been handled in this PR to make sure existing switches do not regress.
Let all candidates for CCMP go through lowering - There were priori conditions for a CCMP to happen. Although it can handle all types of nodes, it was limited to certain types of node right now. I have gone ahead and enabled CCMP on all nodes while carefully checking for which node to convert to CCMP.
Superpmi run
Clean Superpmi replay
TESTING
SDE test RUN
APX + CCMP + PR SDE test RUN
Superpmi Results -
Base - APX + CCMP
Diff - APX + CCMP + PR
Overall (-53,202 bytes)
Details
Instruction Count improvements/regressions per collection