JIT: Use successor edges instead of block targets for BBJ_SWITCH #98671
Conversation
This file contains 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
ghost
assigned 
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsPart of #93020. Updates
|
51 tasks
Diff results for #98671Assembly diffsAssembly diffs for linux/arm ran on linux/x86Diffs are based on 2,250,512 contexts (832,197 MinOpts, 1,418,315 FullOpts). MISSED contexts: 73,582 (3.17%) Overall (-18 bytes)
FullOpts (-18 bytes)
Assembly diffs for windows/x86 ran on linux/x86Diffs are based on 2,354,252 contexts (851,840 MinOpts, 1,502,412 FullOpts). Overall (+2 bytes)
FullOpts (+2 bytes)
Details here Assembly diffs for osx/arm64 ran on linux/x64Diffs are based on 2,293,443 contexts (933,876 MinOpts, 1,359,567 FullOpts). Overall (-20 bytes)
FullOpts (-20 bytes)
Details here Throughput diffsThroughput diffs for linux/arm64 ran on windows/x64Overall (-0.01% to +0.00%)
MinOpts (-0.18% to +0.00%)
FullOpts (-0.01% to +0.00%)
Throughput diffs for linux/x64 ran on windows/x64Overall (-0.01% to +0.00%)
MinOpts (-0.26% to +0.00%)
FullOpts (-0.01% to -0.00%)
Throughput diffs for osx/arm64 ran on windows/x64Overall (-0.01% to +0.00%)
MinOpts (-0.20% to +0.00%)
FullOpts (-0.01% to +0.00%)
Throughput diffs for windows/arm64 ran on windows/x64Overall (-0.01% to +0.00%)
MinOpts (-0.19% to +0.00%)
FullOpts (-0.01% to +0.00%)
Throughput diffs for windows/x64 ran on windows/x64Overall (-0.01% to +0.00%)
MinOpts (-0.26% to +0.00%)
FullOpts (-0.01% to +0.00%)
Details here Throughput diffs for linux/arm ran on windows/x86MinOpts (-0.16% to +0.00%)
Throughput diffs for windows/x86 ran on windows/x86Overall (-0.01% to -0.00%)
MinOpts (-0.27% to +0.01%)
FullOpts (-0.01% to -0.00%)
Details here Throughput diffs for linux/arm64 ran on linux/x64Overall (-0.01% to -0.00%)
MinOpts (-0.19% to 0.00%)
FullOpts (-0.01% to -0.00%)
Details here |
Diff results for #98671Assembly diffsAssembly diffs for linux/arm64 ran on windows/x64Diffs are based on 2,544,350 contexts (1,012,496 MinOpts, 1,531,854 FullOpts). Overall (-140 bytes)
FullOpts (-140 bytes)
Assembly diffs for linux/x64 ran on windows/x64Diffs are based on 2,535,371 contexts (984,668 MinOpts, 1,550,703 FullOpts). Overall (-134 bytes)
FullOpts (-134 bytes)
Assembly diffs for windows/arm64 ran on windows/x64Diffs are based on 2,376,931 contexts (945,150 MinOpts, 1,431,781 FullOpts). MISSED contexts: 5 (0.00%) Overall (-20 bytes)
FullOpts (-20 bytes)
Assembly diffs for windows/x64 ran on windows/x64Diffs are based on 2,416,976 contexts (937,071 MinOpts, 1,479,905 FullOpts). Overall (-59 bytes)
FullOpts (-59 bytes)
Details here Throughput diffsThroughput diffs for linux/x64 ran on linux/x64Overall (-0.01% to -0.00%)
MinOpts (-0.25% to 0.00%)
FullOpts (-0.01% to -0.00%)
Details here |
|
cc @dotnet/jit-contrib, @AndyAyersMS PTAL. Small diffs from changes in edge weights/likelihoods. |
AndyAyersMS
approved these changes
Feb 20, 2024
src/coreclr/jit/importer.cpp
Outdated
| // Remove 'block' from the predecessor list of 'curJump' | ||
| fgRemoveRefPred(curJump, block); | ||
| // Remove 'curEdge' | ||
| fgRemoveRefPred(curEdge->getDestinationBlock(), block); |
There was a problem hiding this comment.
We should add a FlowEdge* overload for the self-removal case of fgRemoveRefPred, since the first thing it does is call fgGetPredForBlock to rediscover the edge we already had.
Likewise fgRemoveBlockAsPred can avoid edge searches in some cases.
There was a problem hiding this comment.
Thanks for pointing that instance out. I added an overloaded fgRemoveRefPred implementation that takes the FlowEdge* and doesn't call fgGetPredForBlock unless the edge's dup count hits zero, and we need to splice it out of the succ block's pred list. Looks like I missed a spot; I'll fix this now.
src/coreclr/jit/compiler.h
Outdated
| @@ -6862,7 +6862,7 @@ class Compiler | |||
| bool optExtractInitTestIncr( | |||
| BasicBlock** pInitBlock, BasicBlock* bottom, BasicBlock* top, GenTree** ppInit, GenTree** ppTest, GenTree** ppIncr); | |||
|
|
|||
| void optRedirectBlock(BasicBlock* blk, | |||
| void optInitDuplicatedBlockTargets(BasicBlock* blk, | |||
There was a problem hiding this comment.
Not sold on the name -- optSetMappedBlockTargets?
But we can sort it out later.
There was a problem hiding this comment.
@jakobbotsch how do you feel about optSetMappedBlockTargets?
There was a problem hiding this comment.
Thanks, I'll fix it now then
Diff results for #98671Assembly diffsAssembly diffs for linux/arm ran on windows/x86Diffs are based on 2,270,268 contexts (836,977 MinOpts, 1,433,291 FullOpts). MISSED contexts: 75,116 (3.20%) Overall (-12 bytes)
FullOpts (-12 bytes)
Assembly diffs for windows/x86 ran on windows/x86Diffs are based on 2,350,557 contexts (850,572 MinOpts, 1,499,985 FullOpts). MISSED contexts: 2,848 (0.12%) Overall (+5 bytes)
FullOpts (+5 bytes)
Details here Assembly diffs for linux/arm64 ran on linux/x64Diffs are based on 2,552,933 contexts (1,022,261 MinOpts, 1,530,672 FullOpts). MISSED contexts: 1,300 (0.05%) Overall (-24 bytes)
FullOpts (-24 bytes)
Assembly diffs for linux/x64 ran on linux/x64Diffs are based on 2,540,651 contexts (986,212 MinOpts, 1,554,439 FullOpts). MISSED contexts: 1,316 (0.05%) Overall (+181 bytes)
FullOpts (+181 bytes)
Assembly diffs for osx/arm64 ran on linux/x64Diffs are based on 2,308,638 contexts (937,510 MinOpts, 1,371,128 FullOpts). MISSED contexts: 1,132 (0.05%) Overall (-24 bytes)
FullOpts (-24 bytes)
Assembly diffs for windows/arm64 ran on linux/x64Diffs are based on 2,387,065 contexts (950,047 MinOpts, 1,437,018 FullOpts). MISSED contexts: 1,169 (0.05%) Overall (-32 bytes)
FullOpts (-32 bytes)
Assembly diffs for windows/x64 ran on linux/x64Diffs are based on 2,558,349 contexts (1,011,639 MinOpts, 1,546,710 FullOpts). MISSED contexts: 3,924 (0.15%) Overall (+80 bytes)
FullOpts (+80 bytes)
Details here Throughput diffsThroughput diffs for linux/arm64 ran on windows/x64Overall (-0.01% to +0.00%)
MinOpts (-0.19% to +0.00%)
FullOpts (-0.01% to +0.00%)
Throughput diffs for linux/x64 ran on windows/x64Overall (-0.01% to +0.00%)
MinOpts (-0.25% to +0.00%)
FullOpts (-0.01% to -0.00%)
Throughput diffs for osx/arm64 ran on windows/x64Overall (-0.01% to +0.00%)
MinOpts (-0.20% to +0.00%)
FullOpts (-0.01% to +0.00%)
Throughput diffs for windows/arm64 ran on windows/x64Overall (-0.01% to +0.00%)
MinOpts (-0.19% to +0.00%)
FullOpts (-0.01% to +0.00%)
Throughput diffs for windows/x64 ran on windows/x64Overall (-0.02% to -0.01%)
MinOpts (-0.27% to +0.00%)
FullOpts (-0.02% to -0.01%)
Details here Throughput diffs for linux/arm ran on windows/x86MinOpts (-0.16% to +0.00%)
Throughput diffs for windows/x86 ran on windows/x86Overall (-0.01% to -0.00%)
MinOpts (-0.27% to +0.00%)
FullOpts (-0.01% to -0.00%)
Details here Throughput diffs for linux/arm64 ran on linux/x64Overall (-0.01% to -0.00%)
MinOpts (-0.19% to 0.00%)
FullOpts (-0.01% to -0.00%)
Throughput diffs for linux/x64 ran on linux/x64Overall (-0.01% to -0.00%)
MinOpts (-0.24% to 0.00%)
FullOpts (-0.01% to -0.00%)
Details here |
Diff results for #98671Assembly diffsAssembly diffs for linux/arm ran on windows/x86Diffs are based on 2,270,268 contexts (836,977 MinOpts, 1,433,291 FullOpts). MISSED contexts: 75,116 (3.20%) Overall (-12 bytes)
FullOpts (-12 bytes)
Details here Assembly diffs for linux/arm64 ran on windows/x64Diffs are based on 2,552,933 contexts (1,022,261 MinOpts, 1,530,672 FullOpts). MISSED contexts: 1,300 (0.05%) Overall (-24 bytes)
FullOpts (-24 bytes)
Assembly diffs for linux/x64 ran on windows/x64Diffs are based on 2,540,651 contexts (986,212 MinOpts, 1,554,439 FullOpts). MISSED contexts: 1,316 (0.05%) Overall (+181 bytes)
FullOpts (+181 bytes)
Details here Throughput diffsThroughput diffs for linux/arm64 ran on linux/x64Overall (-0.01% to -0.00%)
MinOpts (-0.19% to 0.00%)
FullOpts (-0.01% to -0.00%)
Details here |
yurai007
added a commit
to yurai007/runtime
that referenced
this pull request
Feb 27, 2024
After dotnet#98671 we noticed that some of coreclr tests started crashing because of infinite recursion seen like: root@3b821d4a686a:/runtime/artifacts/tests/coreclr/linux.riscv64.Checked/CoreMangLib/system/buffer/ASURT_99893# $CORE_ROOT/corerun -p System.Reflection.Metadata.MetadataUpdater.IsSupported=false -p System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization=true ASURT_99893.dll '' Process terminated. Encountered infinite recursion while looking up resource 'Arg_ResourceFileUnsupportedVersion' in System.Private.CoreLib. Verify the installation of .NET is complete and does not need repairing, and that the state of the process has not become corrupted. at System.Environment.FailFast(System.String) at System.SR.InternalGetResourceString(System.String) at System.SR.GetResourceString(System.String) at System.SR.get_Arg_ResourceFileUnsupportedVersion() at System.Resources.ResourceReader._ReadResources() at System.Resources.ResourceReader.ReadResources() at System.Resources.ResourceReader..ctor(System.IO.Stream, System.Collections.Generic.Dictionary`2<System.String,System.Resources.ResourceLocator>, Boolean) at System.Resources.RuntimeResourceSet..ctor(System.IO.Stream, Boolean) at System.Resources.ManifestBasedResourceGroveler.CreateResourceSet(System.IO.Stream, System.Reflection.Assembly) at System.Resources.ManifestBasedResourceGroveler.GrovelForResourceSet(System.Globalization.CultureInfo, System.Collections.Generic.Dictionary`2<System.String,System.Resources.ResourceSet>, Boolean, Boolean) at System.Resources.ResourceManager.InternalGetResourceSet(System.Globalization.CultureInfo, Boolean, Boolean) at System.Resources.ResourceManager.GetString(System.String, System.Globalization.CultureInfo) at System.SR.InternalGetResourceString(System.String) at System.SR.GetResourceString(System.String) at System.SR.get_Arg_ResourceFileUnsupportedVersion() at System.Resources.ResourceReader._ReadResources() at System.Resources.ResourceReader.ReadResources() at System.Resources.ResourceReader..ctor(System.IO.Stream, System.Collections.Generic.Dictionary`2<System.String,System.Resources.ResourceLocator>, Boolean) at System.Resources.RuntimeResourceSet..ctor(System.IO.Stream, Boolean) at System.Resources.ManifestBasedResourceGroveler.CreateResourceSet(System.IO.Stream, System.Reflection.Assembly) at System.Resources.ManifestBasedResourceGroveler.GrovelForResourceSet(System.Globalization.CultureInfo, System.Collections.Generic.Dictionary`2<System.String,System.Resources.ResourceSet>, Boolean, Boolean) This change fixes issue by adding missing jumpTable increment.
jakobbotsch
pushed a commit
that referenced
this pull request
Feb 28, 2024
After #98671 we noticed that some of coreclr tests started crashing because of infinite recursion seen like: root@3b821d4a686a:/runtime/artifacts/tests/coreclr/linux.riscv64.Checked/CoreMangLib/system/buffer/ASURT_99893# $CORE_ROOT/corerun -p System.Reflection.Metadata.MetadataUpdater.IsSupported=false -p System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization=true ASURT_99893.dll '' Process terminated. Encountered infinite recursion while looking up resource 'Arg_ResourceFileUnsupportedVersion' in System.Private.CoreLib. Verify the installation of .NET is complete and does not need repairing, and that the state of the process has not become corrupted. at System.Environment.FailFast(System.String) at System.SR.InternalGetResourceString(System.String) at System.SR.GetResourceString(System.String) at System.SR.get_Arg_ResourceFileUnsupportedVersion() at System.Resources.ResourceReader._ReadResources() at System.Resources.ResourceReader.ReadResources() at System.Resources.ResourceReader..ctor(System.IO.Stream, System.Collections.Generic.Dictionary`2<System.String,System.Resources.ResourceLocator>, Boolean) at System.Resources.RuntimeResourceSet..ctor(System.IO.Stream, Boolean) at System.Resources.ManifestBasedResourceGroveler.CreateResourceSet(System.IO.Stream, System.Reflection.Assembly) at System.Resources.ManifestBasedResourceGroveler.GrovelForResourceSet(System.Globalization.CultureInfo, System.Collections.Generic.Dictionary`2<System.String,System.Resources.ResourceSet>, Boolean, Boolean) at System.Resources.ResourceManager.InternalGetResourceSet(System.Globalization.CultureInfo, Boolean, Boolean) at System.Resources.ResourceManager.GetString(System.String, System.Globalization.CultureInfo) at System.SR.InternalGetResourceString(System.String) at System.SR.GetResourceString(System.String) at System.SR.get_Arg_ResourceFileUnsupportedVersion() at System.Resources.ResourceReader._ReadResources() at System.Resources.ResourceReader.ReadResources() at System.Resources.ResourceReader..ctor(System.IO.Stream, System.Collections.Generic.Dictionary`2<System.String,System.Resources.ResourceLocator>, Boolean) at System.Resources.RuntimeResourceSet..ctor(System.IO.Stream, Boolean) at System.Resources.ManifestBasedResourceGroveler.CreateResourceSet(System.IO.Stream, System.Reflection.Assembly) at System.Resources.ManifestBasedResourceGroveler.GrovelForResourceSet(System.Globalization.CultureInfo, System.Collections.Generic.Dictionary`2<System.String,System.Resources.ResourceSet>, Boolean, Boolean) This change fixes issue by adding missing jumpTable increment.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Part of #93020. Updates
BBswtDesc::bbsDstTabto use successor edges instead of block targets for switch blocks. Also renamesoptRedirectBlocktooptInitDuplicatedBlockTargetsbased on conversation here.