From dd2468fc835147ab922a2d027dfe0e83bf3fef8a Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 9 Dec 2024 14:23:48 +0100 Subject: [PATCH 1/5] JIT: Include more edges in `BlockDominancePreds` Because of spurious flow it is possible that the preds of the try-begin block are not the only blocks that can dominate a handler. We handled this possibility, but only for finally/fault blocks that can directly have these edges. However, even other handler blocks can be reachable through spurious paths that involves finally/fault blocks, and in these cases returning the preds of the try-begin block is not enough to compute the right dominator statically. --- src/coreclr/jit/block.cpp | 50 ++++++++++++--------------------------- 1 file changed, 15 insertions(+), 35 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index be2ba15e254690..8144388a5a3657 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -267,15 +267,21 @@ FlowEdge* Compiler::BlockPredsWithEH(BasicBlock* blk) // 'blk'. // // Arguments: -// blk - Block to get dominance predecessors for. +// blk - Block to get dominance predecessors for. // // Returns: -// List of edges. +// List of edges. // // Remarks: -// Differs from BlockPredsWithEH only in the treatment of handler blocks; -// enclosed blocks are never dominance preds, while all predecessors of -// blocks in the 'try' are (currently only the first try block expected). +// Differs from BlockPredsWithEH only in the treatment of handler blocks; +// enclosed blocks are never dominance preds, while all predecessors of +// blocks in the 'try' are (currently only the first try block expected). +// +// There are additional complications due to spurious flow because of +// two-pass EH. In the flow graph with EH edges we can see entries into the +// try from filters outside the try, to blocks other than the "try-begin" +// block. Hence we need to consider the full set of blocks in the try region +// when considering the block dominance preds. // FlowEdge* Compiler::BlockDominancePreds(BasicBlock* blk) { @@ -284,14 +290,6 @@ FlowEdge* Compiler::BlockDominancePreds(BasicBlock* blk) return blk->bbPreds; } - EHblkDsc* ehblk = ehGetBlockHndDsc(blk); - if (!ehblk->HasFinallyOrFaultHandler() || (ehblk->ebdHndBeg != blk)) - { - return ehblk->ebdTryBeg->bbPreds; - } - - // Finally/fault handlers can be preceded by enclosing filters due to 2 - // pass EH, so add those and keep them cached. BlockToFlowEdgeMap* domPreds = GetDominancePreds(); FlowEdge* res; if (domPreds->Lookup(blk, &res)) @@ -299,29 +297,11 @@ FlowEdge* Compiler::BlockDominancePreds(BasicBlock* blk) return res; } - res = ehblk->ebdTryBeg->bbPreds; - if (ehblk->HasFinallyOrFaultHandler() && (ehblk->ebdHndBeg == blk)) + EHblkDsc* ehblk = ehGetBlockHndDsc(blk); + res = BlockPredsWithEH(blk); + for (BasicBlock* predBlk : ehblk->ebdTryBeg->PredBlocks()) { - // block is a finally or fault handler; all enclosing filters are predecessors - unsigned enclosing = ehblk->ebdEnclosingTryIndex; - while (enclosing != EHblkDsc::NO_ENCLOSING_INDEX) - { - EHblkDsc* enclosingDsc = ehGetDsc(enclosing); - if (enclosingDsc->HasFilter()) - { - for (BasicBlock* filterBlk = enclosingDsc->ebdFilter; filterBlk != enclosingDsc->ebdHndBeg; - filterBlk = filterBlk->Next()) - { - res = new (this, CMK_FlowEdge) FlowEdge(filterBlk, blk, res); - - assert(filterBlk->VisitEHEnclosedHandlerSecondPassSuccs(this, [blk](BasicBlock* succ) { - return succ == blk ? BasicBlockVisit::Abort : BasicBlockVisit::Continue; - }) == BasicBlockVisit::Abort); - } - } - - enclosing = enclosingDsc->ebdEnclosingTryIndex; - } + res = new (this, CMK_FlowEdge) FlowEdge(predBlk, blk, res); } domPreds->Set(blk, res); From 4d19675f72ffdbf166ca424837a1d746698eea41 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 9 Dec 2024 14:57:57 +0100 Subject: [PATCH 2/5] Add test --- .../JitBlue/Runtime_109981/Runtime_109981.cs | 66 +++++++++++++++++++ .../Runtime_109981/Runtime_109981.csproj | 8 +++ 2 files changed, 74 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.cs b/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.cs new file mode 100644 index 00000000000000..fe06dcddc95837 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.cs @@ -0,0 +1,66 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; +using Xunit; + +public class Runtime_109981 +{ + [Fact] + public static void TestEntryPoint() + { + Foo(14); + } + + public static int Foo(int x) + { + if (x == 123) + return 0; + + int sum = 0; + for (int i = 0; i < x; i++) + { + sum += i; + } + + try + { + if (x != 123) + return sum; + + try + { + try + { + Bar(); + } + finally + { + sum += 9; + } + } + catch (ArgumentException) + { + sum += 1000; + } + } + catch when (Filter()) + { + sum += 10000; + } + + return sum; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void Bar() + { + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool Filter() + { + return true; + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.csproj new file mode 100644 index 00000000000000..de6d5e08882e86 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.csproj @@ -0,0 +1,8 @@ + + + True + + + + + From b3b3715e11024b26a8588d53e6b917f267f6028f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 9 Dec 2024 14:59:58 +0100 Subject: [PATCH 3/5] Call function in test --- .../JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.cs b/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.cs index fe06dcddc95837..b13a0ad113ec83 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.cs @@ -8,10 +8,7 @@ public class Runtime_109981 { [Fact] - public static void TestEntryPoint() - { - Foo(14); - } + public static int TestEntryPoint() => Foo(14); public static int Foo(int x) { From c7a6eb519c16489f8f82106d929278628cd1a5e7 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 9 Dec 2024 15:09:07 +0100 Subject: [PATCH 4/5] Run jit-format --- src/coreclr/jit/block.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 8144388a5a3657..b00dc1282a2900 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -298,7 +298,7 @@ FlowEdge* Compiler::BlockDominancePreds(BasicBlock* blk) } EHblkDsc* ehblk = ehGetBlockHndDsc(blk); - res = BlockPredsWithEH(blk); + res = BlockPredsWithEH(blk); for (BasicBlock* predBlk : ehblk->ebdTryBeg->PredBlocks()) { res = new (this, CMK_FlowEdge) FlowEdge(predBlk, blk, res); From 269d3eafbebac229fe72a01fbfc4558f5bcfed1b Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 9 Dec 2024 17:05:47 +0100 Subject: [PATCH 5/5] Fix test --- .../Regression/JitBlue/Runtime_109981/Runtime_109981.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.cs b/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.cs index b13a0ad113ec83..a7c8bcd92d81aa 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_109981/Runtime_109981.cs @@ -15,7 +15,7 @@ public static int Foo(int x) if (x == 123) return 0; - int sum = 0; + int sum = 9; for (int i = 0; i < x; i++) { sum += i; @@ -34,17 +34,17 @@ public static int Foo(int x) } finally { - sum += 9; + sum += 1000; } } catch (ArgumentException) { - sum += 1000; + sum += 10000; } } catch when (Filter()) { - sum += 10000; + sum += 100000; } return sum;