From 65cc982554e2f90297f1d516284bdcde7c6724ea Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 20 Sep 2021 10:59:58 -0700 Subject: [PATCH 1/4] Include BBJ_ALWAYS in inverse post ordering for computing reachability --- src/coreclr/jit/fgopt.cpp | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index c6f6bc8dd89cda..1a8fa855a65724 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -636,22 +636,7 @@ void Compiler::fgDfsInvPostOrder() // an incoming edge into the block). assert(fgEnterBlksSetValid); -#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) - // - // BlockSetOps::UnionD(this, startNodes, fgEnterBlks); - // - // This causes problems on ARM, because we for BBJ_CALLFINALLY/BBJ_ALWAYS pairs, we add the BBJ_ALWAYS - // to the enter blocks set to prevent flow graph optimizations from removing it and creating retless call finallies - // (BBF_RETLESS_CALL). This leads to an incorrect DFS ordering in some cases, because we start the recursive walk - // from the BBJ_ALWAYS, which is reachable from other blocks. A better solution would be to change ARM to avoid - // creating retless calls in a different way, not by adding BBJ_ALWAYS to fgEnterBlks. - // - // So, let us make sure at least fgFirstBB is still there, even if it participates in a loop. - BlockSetOps::AddElemD(this, startNodes, 1); - assert(fgFirstBB->bbNum == 1); -#else BlockSetOps::UnionD(this, startNodes, fgEnterBlks); -#endif assert(BlockSetOps::IsMember(this, startNodes, fgFirstBB->bbNum)); From 134ab01fd7d462331e07fdcb114947dd838cf3e4 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 20 Sep 2021 12:33:00 -0700 Subject: [PATCH 2/4] Add test case --- .../JitBlue/Runtime_59298/Runtime_59298.cs | 66 +++++++++++++++++++ .../Runtime_59298/Runtime_59298.csproj | 12 ++++ 2 files changed, 78 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_59298/Runtime_59298.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_59298/Runtime_59298.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_59298/Runtime_59298.cs b/src/tests/JIT/Regression/JitBlue/Runtime_59298/Runtime_59298.cs new file mode 100644 index 00000000000000..1458bd4dfe47f1 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_59298/Runtime_59298.cs @@ -0,0 +1,66 @@ +using System.Runtime.CompilerServices; +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// +// Note: In below test case, we do not iterate over BBJ_ALWAYS blocks while computing the +// reachability leading to assert +public class Runtime_59298 +{ + public struct S2 + { + public struct S2_D1_F1 + { + public double double_1; + } + } + static int s_int_6 = -2; + static S2 s_s2_16 = new S2(); + int int_6 = -2; + + [MethodImpl(MethodImplOptions.NoInlining)] + public int LeafMethod6() + { + return 1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public S2 Method0(out short p_short_1) + { + long long_7 = -1; + p_short_1 = 0; + switch (long_7) + { + case -5: + { + do + { + try + { + int_6 ^= int_6; + } + finally + { + // The expression doesn't matter, it just has to be long enough + // to have few extra blocks which we don't walk when doing inverse + // post order while computing dominance information. + long_7 &= long_7; + int_6 &= (int_6 /= (int_6 -= LeafMethod6() - int_6) + 69) / ((int_6 << (int_6 - int_6)) + (int_6 |= LeafMethod6()) + (LeafMethod6() >> s_int_6) + 62); + } + } + while (long_7 == 8); + break; + } + default: + { + break; + } + } + return s_s2_16; + } + + public static int Main(string[] args) + { + new Runtime_59298().Method0(out short s); + return s + 100; + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_59298/Runtime_59298.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_59298/Runtime_59298.csproj new file mode 100644 index 00000000000000..f3e1cbd44b4041 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_59298/Runtime_59298.csproj @@ -0,0 +1,12 @@ + + + Exe + + + None + True + + + + + From 49dcb7085a277c3af897a644a3e451a1d029912f Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 27 Sep 2021 23:54:13 -0700 Subject: [PATCH 3/4] Add BBJ_ALWAYS in fgAlways list --- src/coreclr/jit/compiler.h | 4 ++++ src/coreclr/jit/fgbasic.cpp | 4 ++++ src/coreclr/jit/fgopt.cpp | 24 +++++++++++++++--------- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 351e49636dfded..22d904e7ab3801 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4985,6 +4985,10 @@ class Compiler BlockSet fgEnterBlks; // Set of blocks which have a special transfer of control; the "entry" blocks plus EH handler // begin blocks. +#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + BlockSet fgAlwaysBlks; // Set of blocks which are BBJ_ALWAYS part of BBJ_CALLFINALLY/BBJ_ALWAYS pair that should never + // be removed due to a requirement to use the BBJ_ALWAYS for generating code and not have "retless" blocks. +#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) #ifdef DEBUG bool fgReachabilitySetsValid; // Are the bbReach sets valid? diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index f3d48c8d338690..5dbf989ff86fa1 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -119,6 +119,10 @@ void Compiler::fgInit() /* This is set by fgComputeReachability */ fgEnterBlks = BlockSetOps::UninitVal(); +#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + fgAlwaysBlks = BlockSetOps::UninitVal(); +#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + #ifdef DEBUG fgEnterBlksSetValid = false; #endif // DEBUG diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 1a8fa855a65724..98ed38531a6353 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -297,6 +297,10 @@ void Compiler::fgComputeEnterBlocksSet() fgEnterBlks = BlockSetOps::MakeEmpty(this); +#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + fgAlwaysBlks = BlockSetOps::MakeEmpty(this); +#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + /* Now set the entry basic block */ BlockSetOps::AddElemD(this, fgEnterBlks, fgFirstBB->bbNum); assert(fgFirstBB->bbNum == 1); @@ -315,19 +319,15 @@ void Compiler::fgComputeEnterBlocksSet() } #if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) - // TODO-ARM-Cleanup: The ARM code here to prevent creating retless calls by adding the BBJ_ALWAYS - // to the enter blocks is a bit of a compromise, because sometimes the blocks are already reachable, - // and it messes up DFS ordering to have them marked as enter block. We should prevent the - // creation of retless calls some other way. + // For ARM code, prevent creating retless calls by adding the BBJ_ALWAYS to the "fgAlwaysBlks" list. for (BasicBlock* const block : Blocks()) { if (block->bbJumpKind == BBJ_CALLFINALLY) { assert(block->isBBCallAlwaysPair()); - - // Don't remove the BBJ_ALWAYS block that is only here for the unwinder. It might be dead - // if the finally is no-return, so mark it as an entry point. - BlockSetOps::AddElemD(this, fgEnterBlks, block->bbNext->bbNum); + + // Don't remove the BBJ_ALWAYS block that is only here for the unwinder. + BlockSetOps::AddElemD(this, fgAlwaysBlks, block->bbNext->bbNum); } } #endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) @@ -401,6 +401,13 @@ bool Compiler::fgRemoveUnreachableBlocks() { goto SKIP_BLOCK; } + +#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + if (!BlockSetOps::IsEmptyIntersection(this, fgAlwaysBlks, block->bbReach)) + { + goto SKIP_BLOCK; + } +#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) } // Remove all the code for the block @@ -637,7 +644,6 @@ void Compiler::fgDfsInvPostOrder() assert(fgEnterBlksSetValid); BlockSetOps::UnionD(this, startNodes, fgEnterBlks); - assert(BlockSetOps::IsMember(this, startNodes, fgFirstBB->bbNum)); // Call the flowgraph DFS traversal helper. From 72342c070bd021b7cc07b7bf8600d9d02a2a6f10 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Tue, 28 Sep 2021 09:49:10 -0700 Subject: [PATCH 4/4] jit format --- src/coreclr/jit/compiler.h | 6 ++++-- src/coreclr/jit/fgopt.cpp | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 22d904e7ab3801..4e1c25b53520d3 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4986,8 +4986,10 @@ class Compiler BlockSet fgEnterBlks; // Set of blocks which have a special transfer of control; the "entry" blocks plus EH handler // begin blocks. #if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) - BlockSet fgAlwaysBlks; // Set of blocks which are BBJ_ALWAYS part of BBJ_CALLFINALLY/BBJ_ALWAYS pair that should never - // be removed due to a requirement to use the BBJ_ALWAYS for generating code and not have "retless" blocks. + BlockSet fgAlwaysBlks; // Set of blocks which are BBJ_ALWAYS part of BBJ_CALLFINALLY/BBJ_ALWAYS pair that should + // never be removed due to a requirement to use the BBJ_ALWAYS for generating code and + // not have "retless" blocks. + #endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) #ifdef DEBUG diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 98ed38531a6353..71daf460ca69f1 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -325,7 +325,7 @@ void Compiler::fgComputeEnterBlocksSet() if (block->bbJumpKind == BBJ_CALLFINALLY) { assert(block->isBBCallAlwaysPair()); - + // Don't remove the BBJ_ALWAYS block that is only here for the unwinder. BlockSetOps::AddElemD(this, fgAlwaysBlks, block->bbNext->bbNum); }