JIT: Fix handling of filter successors #89275
Merged
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
Filters are run during the first pass of EH which makes determining their successors complicated, and the JIT did not get this right. In particular, after running filters as part of first-pass EH, control may flow to any enclosed finally or fault handler as part of second-pass EH. Thus, these should be considered as EH successors. This adds a BasicBlock::VisitEHSecondPassSuccs to visit these successors. The logic was mostly extracted from liveness that (almost) got it right: there was a condition that meant the logic did not run for top-level try-filter clauses. This change folds in Bruce's change to fix this scenario as well. There was one more bug in the logic in liveness: to determine the enclosed fault/finally blocks, the logic iterates backwards in the EH table while looking for enclosed clauses. However, this was only checking for clauses enclosed in the try region when it should also check for clauses enclosed in the handler region. Fix dotnet#86538 Fix dotnet#88168 Co-authored-by: Bruce Forstall <brucefo@microsoft.com>
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsFilters are run during the first pass of EH which makes determining their successors complicated, and the JIT did not get this right. In particular, after running filters as part of first-pass EH, control may flow to any enclosed finally or fault handler as part of second-pass EH. Thus, these should be considered as EH successors. This adds a BasicBlock::VisitEHSecondPassSuccs to visit these successors. The logic was mostly extracted from liveness that (almost) got it right: there was a condition that meant the logic did not run for top-level try-filter clauses. This change folds in Bruce's change to fix this scenario as well. There was one more bug in the logic in liveness: to determine the enclosed fault/finally blocks, the logic iterates backwards in the EH table while looking for enclosed clauses. However, this was only checking for clauses enclosed in the try region when it should also check for clauses enclosed in the handler region.
|
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Leave it up to the GCStress jobs instead. Some platforms do not support GCStress.
|
There is still a TP regression for linux. The detailed stats look like: Base: 57696352942, Diff: 57808766749, +0.1948%
BasicBlockVisit VisitEHSuccessors<LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool)::'lambda'(BasicBlock*)>(Compiler*, BasicBlock*, LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool)::'lambda'(BasicBlock*)) : 114296864 : NA : 22.04% : +0.1981%
Compiler::fgAddHandlerLiveVars(BasicBlock*, unsigned long*&) : 45110935 : NA : 8.70% : +0.0782%
BasicBlockVisit VisitEHSuccessors<Compiler::optReachable(BasicBlock*, BasicBlock*, BasicBlock*)::$_0>(Compiler*, BasicBlock*, Compiler::optReachable(BasicBlock*, BasicBlock*, BasicBlock*)::$_0) : 33963609 : NA : 6.55% : +0.0589%
BasicBlockVisit VisitEHSuccessors<SsaBuilder::AddPhiArgsToSuccessors(BasicBlock*)::$_1>(Compiler*, BasicBlock*, SsaBuilder::AddPhiArgsToSuccessors(BasicBlock*)::$_1) : 16226626 : NA : 3.13% : +0.0281%
BasicBlockVisit VisitEHSuccessors<ValueNumberState::FinishVisit(BasicBlock*)::'lambda'(BasicBlock*)>(Compiler*, BasicBlock*, ValueNumberState::FinishVisit(BasicBlock*)::'lambda'(BasicBlock*)) : 15772530 : NA : 3.04% : +0.0273%
BasicBlockVisit VisitEHSuccessors<void DataFlow::ForwardAnalysis<AssertionPropFlowCallback>(AssertionPropFlowCallback&)::'lambda'(BasicBlock*)>(Compiler*, BasicBlock*, AssertionPropFlowCallback) : 15625325 : NA : 3.01% : +0.0271%
BasicBlockVisit VisitEHSuccessors<AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler*, BasicBlock*)::$_0>(Compiler*, BasicBlock*, AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler*, BasicBlock*)::$_0) : 14836539 : NA : 2.86% : +0.0257%
BasicBlockVisit VisitEHSuccessors<void DataFlow::ForwardAnalysis<CSE_DataFlow>(CSE_DataFlow&)::'lambda'(BasicBlock*)>(Compiler*, BasicBlock*, CSE_DataFlow) : 11960412 : NA : 2.31% : +0.0207%
BasicBlockVisit BasicBlock::VisitEHSecondPassSuccs<LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool)::'lambda'(BasicBlock*)>(Compiler*, LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool)::'lambda'(BasicBlock*)) : 10856173 : NA : 2.09% : +0.0188%
BasicBlockVisit VisitEHSuccessors<PromotionLiveness::PerBlockLiveness(BasicBlock*)::$_0>(Compiler*, BasicBlock*, PromotionLiveness::PerBlockLiveness(BasicBlock*)::$_0) : 5703223 : NA : 1.10% : +0.0099%
Compiler::bbInFilterBBRange(BasicBlock*) : 5138265 : NA : 0.99% : +0.0089%
BasicBlockVisit VisitEHSuccessors<void LinearScan::buildIntervals<true>()::'lambda'(BasicBlock*)>(Compiler*, BasicBlock*, true) : 4811984 : NA : 0.93% : +0.0083%
Compiler::BlockPredsWithEH(BasicBlock*) : 4334425 : +2.56% : 0.84% : +0.0075%
LiveVarAnalysis::LiveVarAnalysis(Compiler*) : 3271145 : NA : 0.63% : +0.0057%
BasicBlockVisit BasicBlock::VisitAllSuccs<LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool)::'lambda'(BasicBlock*)>(Compiler*, LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool)::'lambda'(BasicBlock*)) : 2790940 : +0.82% : 0.54% : +0.0048%
EHblkDsc::InFilterRegionBBRange(BasicBlock*) : 2203961 : +856.34% : 0.42% : +0.0038%
BasicBlockVisit BasicBlock::VisitEHSecondPassSuccs<void DataFlow::ForwardAnalysis<AssertionPropFlowCallback>(AssertionPropFlowCallback&)::'lambda'(BasicBlock*)>(Compiler*, AssertionPropFlowCallback) : 1318125 : NA : 0.25% : +0.0023%
BasicBlockVisit BasicBlock::VisitAllSuccs<void LinearScan::buildIntervals<true>()::'lambda'(BasicBlock*)>(Compiler*, true) : 1210747 : +4.75% : 0.23% : +0.0021%
SsaBuilder::AddPhiArg(BasicBlock*, Statement*, GenTreePhi*, unsigned int, unsigned int, BasicBlock*) : 1204341 : +3.19% : 0.23% : +0.0021%
BasicBlockVisit BasicBlock::VisitEHSecondPassSuccs<void DataFlow::ForwardAnalysis<CSE_DataFlow>(CSE_DataFlow&)::'lambda'(BasicBlock*)>(Compiler*, CSE_DataFlow) : 1061830 : NA : 0.20% : +0.0018%
BasicBlockVisit BasicBlock::VisitEHSecondPassSuccs<ValueNumberState::FinishVisit(BasicBlock*)::'lambda'(BasicBlock*)>(Compiler*, ValueNumberState::FinishVisit(BasicBlock*)::'lambda'(BasicBlock*)) : 1043750 : NA : 0.20% : +0.0018%
BasicBlockVisit BasicBlock::VisitAllSuccs<void DataFlow::ForwardAnalysis<AssertionPropFlowCallback>(AssertionPropFlowCallback&)::'lambda'(BasicBlock*)>(Compiler*, AssertionPropFlowCallback) : 650373 : +1.57% : 0.13% : +0.0011%
AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler*, BasicBlock*) : -594474 : -2.03% : 0.11% : -0.0010%
PromotionLiveness::PerBlockLiveness(BasicBlock*) : -690959 : -3.24% : 0.13% : -0.0012%
Compiler::fgLiveVarAnalysis(bool) : -4197657 : -100.00% : 0.81% : -0.0073%
Compiler::fgInterBlockLocalVarLiveness() : -5039714 : -1.68% : 0.97% : -0.0087%
LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool) : -5349213 : -1.03% : 1.03% : -0.0093%
Compiler::optReachable(BasicBlock*, BasicBlock*, BasicBlock*) : -5955380 : -8.63% : 1.15% : -0.0103%
Compiler::fgGetHandlerLiveVars(BasicBlock*) : -48197118 : -100.00% : 9.29% : -0.0835%
Compiler::ehBlockHasExnFlowDsc(BasicBlock*) : -56110722 : -72.16% : 10.82% : -0.0973%
Compiler::ehGetBlockExnFlowDsc(BasicBlock*) : -76371220 : -72.81% : 14.73% : -0.1324%(future note to self: need to unstrip libclrjit.so (or presumably just pass -keepnativesymbols) to get it to work). When looking closer, clang is fully inlining |
|
If I manually add checks for Base: 57696352942, Diff: 57670826699, -0.0442%
BasicBlockVisit VisitEHSuccessors<LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool)::'lambda'(BasicBlock*)>(Compiler*, BasicBlock*, LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool)::'lambda'(BasicBlock*)) : 47713859 : NA : 12.25% : +0.0827%
Compiler::fgAddHandlerLiveVars(BasicBlock*, unsigned long*&) : 45110935 : NA : 11.58% : +0.0782%
BasicBlockVisit BasicBlock::VisitEHSecondPassSuccs<LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool)::'lambda'(BasicBlock*)>(Compiler*, LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool)::'lambda'(BasicBlock*)) : 24743137 : NA : 6.35% : +0.0429%
BasicBlockVisit VisitEHSuccessors<Compiler::optReachable(BasicBlock*, BasicBlock*, BasicBlock*)::$_0>(Compiler*, BasicBlock*, Compiler::optReachable(BasicBlock*, BasicBlock*, BasicBlock*)::$_0) : 13075204 : NA : 3.36% : +0.0227%
BasicBlockVisit VisitEHSuccessors<void DataFlow::ForwardAnalysis<AssertionPropFlowCallback>(AssertionPropFlowCallback&)::'lambda'(BasicBlock*)>(Compiler*, BasicBlock*, AssertionPropFlowCallback) : 5267582 : NA : 1.35% : +0.0091%
Compiler::bbInFilterBBRange(BasicBlock*) : 5138265 : NA : 1.32% : +0.0089%
BasicBlockVisit VisitEHSuccessors<SsaBuilder::AddPhiArgsToSuccessors(BasicBlock*)::$_1>(Compiler*, BasicBlock*, SsaBuilder::AddPhiArgsToSuccessors(BasicBlock*)::$_1) : 4638729 : NA : 1.19% : +0.0080%
Compiler::BlockPredsWithEH(BasicBlock*) : 4334425 : +2.56% : 1.11% : +0.0075%
BasicBlockVisit VisitEHSuccessors<void DataFlow::ForwardAnalysis<CSE_DataFlow>(CSE_DataFlow&)::'lambda'(BasicBlock*)>(Compiler*, BasicBlock*, CSE_DataFlow) : 4217076 : NA : 1.08% : +0.0073%
BasicBlockVisit VisitEHSuccessors<AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler*, BasicBlock*)::$_0>(Compiler*, BasicBlock*, AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler*, BasicBlock*)::$_0) : 4129431 : NA : 1.06% : +0.0072%
BasicBlockVisit BasicBlock::VisitEHSecondPassSuccs<void DataFlow::ForwardAnalysis<AssertionPropFlowCallback>(AssertionPropFlowCallback&)::'lambda'(BasicBlock*)>(Compiler*, AssertionPropFlowCallback) : 3784453 : NA : 0.97% : +0.0066%
LiveVarAnalysis::LiveVarAnalysis(Compiler*) : 3271145 : NA : 0.84% : +0.0057%
BasicBlockVisit BasicBlock::VisitEHSecondPassSuccs<void DataFlow::ForwardAnalysis<CSE_DataFlow>(CSE_DataFlow&)::'lambda'(BasicBlock*)>(Compiler*, CSE_DataFlow) : 3003646 : NA : 0.77% : +0.0052%
BasicBlockVisit BasicBlock::VisitEHSecondPassSuccs<ValueNumberState::FinishVisit(BasicBlock*)::'lambda'(BasicBlock*)>(Compiler*, ValueNumberState::FinishVisit(BasicBlock*)::'lambda'(BasicBlock*)) : 2965178 : NA : 0.76% : +0.0051%
BasicBlockVisit VisitEHSuccessors<PromotionLiveness::PerBlockLiveness(BasicBlock*)::$_0>(Compiler*, BasicBlock*, PromotionLiveness::PerBlockLiveness(BasicBlock*)::$_0) : 2321743 : NA : 0.60% : +0.0040%
EHblkDsc::InFilterRegionBBRange(BasicBlock*) : 2181918 : +847.77% : 0.56% : +0.0038%
BasicBlockVisit VisitEHSuccessors<void LinearScan::buildIntervals<true>()::'lambda'(BasicBlock*)>(Compiler*, BasicBlock*, true) : 1656054 : NA : 0.43% : +0.0029%
SsaBuilder::AddPhiArg(BasicBlock*, Statement*, GenTreePhi*, unsigned int, unsigned int, BasicBlock*) : 1204341 : +3.19% : 0.31% : +0.0021%
BasicBlockVisit BasicBlock::VisitEHSecondPassSuccs<void LinearScan::buildIntervals<true>()::'lambda'(BasicBlock*)>(Compiler*, true) : 1154594 : NA : 0.30% : +0.0020%
BasicBlockVisit BasicBlock::VisitAllSuccs<void LinearScan::buildIntervals<true>()::'lambda'(BasicBlock*)>(Compiler*, true) : 523034 : +2.05% : 0.13% : +0.0009%
SsaBuilder::AddPhiArgsToSuccessors(BasicBlock*) : -686946 : -2.38% : 0.18% : -0.0012%
PromotionLiveness::PerBlockLiveness(BasicBlock*) : -923898 : -4.33% : 0.24% : -0.0016%
BasicBlockVisit BasicBlock::VisitAllSuccs<void DataFlow::ForwardAnalysis<CSE_DataFlow>(CSE_DataFlow&)::'lambda'(BasicBlock*)>(Compiler*, CSE_DataFlow) : -1107936 : -3.45% : 0.28% : -0.0019%
BasicBlockVisit BasicBlock::VisitAllSuccs<void DataFlow::ForwardAnalysis<AssertionPropFlowCallback>(AssertionPropFlowCallback&)::'lambda'(BasicBlock*)>(Compiler*, AssertionPropFlowCallback) : -1369265 : -3.30% : 0.35% : -0.0024%
Compiler::fgLiveVarAnalysis(bool) : -4197657 : -100.00% : 1.08% : -0.0073%
Compiler::fgInterBlockLocalVarLiveness() : -5039714 : -1.68% : 1.29% : -0.0087%
LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool) : -5349213 : -1.03% : 1.37% : -0.0093%
Compiler::optReachable(BasicBlock*, BasicBlock*, BasicBlock*) : -7533498 : -10.92% : 1.93% : -0.0131%
BasicBlockVisit BasicBlock::VisitAllSuccs<LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool)::'lambda'(BasicBlock*)>(Compiler*, LiveVarAnalysis::PerBlockAnalysis(BasicBlock*, bool, bool)::'lambda'(BasicBlock*)) : -14544499 : -4.29% : 3.73% : -0.0252%
Compiler::fgGetHandlerLiveVars(BasicBlock*) : -48197118 : -100.00% : 12.37% : -0.0835%
Compiler::ehBlockHasExnFlowDsc(BasicBlock*) : -56110722 : -72.16% : 14.40% : -0.0973%
Compiler::ehGetBlockExnFlowDsc(BasicBlock*) : -61886436 : -59.00% : 15.89% : -0.1073%However, I'm struggling to get clang to make these choices during inlining. |
|
I think I'm going to leave this as is (with the small TP regression on Linux) -- I think there's an easier way to optimize liveness here -- I see no reason that we even need to visit all successors the way we are doing it from liveness. The successor-EH-successors should be unnecessary because they are already part of the live-in sets of the regular successors, so the fixpoint will take care of those. The normal EH successors are handled by I ran stress jobs above; the failures were
cc @dotnet/jit-contrib PTAL @BruceForstall |
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.
Filters are run during the first pass of EH which makes determining their successors complicated, and the JIT did not get this right. In particular, after running filters as part of first-pass EH, control may flow to any enclosed finally or fault handler as part of second-pass EH. Thus, these should be considered as EH successors. This adds a BasicBlock::VisitEHSecondPassSuccs to visit these successors.
The logic was mostly extracted from liveness that (almost) got it right: there was a condition that meant the logic did not run for top-level try-filter clauses. This change folds in Bruce's change to fix this scenario as well.
There was one more bug in the logic in liveness: to determine the enclosed fault/finally blocks, the logic iterates backwards in the EH table while looking for enclosed clauses. However, this was only checking for clauses enclosed in the try region when it should also check for clauses enclosed in the handler region.
Fix #86538
Fix #88168