From 54cf23c8d0a26e3f2abdc5f87422e52ad93215c8 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 6 Jun 2022 13:15:49 -0700 Subject: [PATCH] Bug fixes from dead code elimination (#69897) * Track if unreachable blocks were detected squash: Update test case name * Unreachable one-by-one unreachable to squash * Add test cases * add test description * Check if there were any unreachable blocks * review feedback * jit format * Fix a case for isBBCallAlwaysPairTail for Arm --- src/coreclr/jit/fgopt.cpp | 36 +++++++++++++-- .../JitBlue/GitHub_69659/GitHub_69659_1.cs | 35 ++++++++++++++ .../GitHub_69659/GitHub_69659_1.csproj | 13 ++++++ .../JitBlue/GitHub_69659/GitHub_69659_2.cs | 46 +++++++++++++++++++ .../GitHub_69659/GitHub_69659_2.csproj | 13 ++++++ 5 files changed, 139 insertions(+), 4 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_1.cs create mode 100644 src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_1.csproj create mode 100644 src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_2.cs create mode 100644 src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_2.csproj diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 228e01cbf5ad4..d0d4ae647ec98 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -654,6 +654,7 @@ bool Compiler::fgRemoveDeadBlocks() JITDUMP("\n*************** In fgRemoveDeadBlocks()"); jitstd::list worklist(jitstd::allocator(getAllocator(CMK_Reachability))); + worklist.push_back(fgFirstBB); // Do not remove handler blocks @@ -713,16 +714,43 @@ bool Compiler::fgRemoveDeadBlocks() } } + // Track if there is any unreachable block. Even if it is marked with + // BBF_DONT_REMOVE, fgRemoveUnreachableBlocks() still removes the code + // inside the block. So this variable tracks if we ever found such blocks + // or not. + bool hasUnreachableBlock = false; + // A block is unreachable if no path was found from // any of the fgFirstBB, handler, filter or BBJ_ALWAYS (Arm) blocks. auto isBlockRemovable = [&](BasicBlock* block) -> bool { - return !BlockSetOps::IsMember(this, visitedBlocks, block->bbNum); + bool isVisited = BlockSetOps::IsMember(this, visitedBlocks, block->bbNum); + bool isRemovable = (!isVisited || block->bbRefs == 0); + +#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + isRemovable &= + !block->isBBCallAlwaysPairTail(); // can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair +#endif + hasUnreachableBlock |= isRemovable; + + return isRemovable; }; - bool changed = fgRemoveUnreachableBlocks(isBlockRemovable); + bool changed = false; + unsigned iterationCount = 1; + do + { + JITDUMP("\nRemoving unreachable blocks for fgRemoveDeadBlocks iteration #%u\n", iterationCount); + + // Just to be paranoid, avoid infinite loops; fall back to minopts. + if (iterationCount++ > 10) + { + noway_assert(!"Too many unreachable block removal loops"); + } + changed = fgRemoveUnreachableBlocks(isBlockRemovable); + } while (changed); #ifdef DEBUG - if (verbose && changed) + if (verbose && hasUnreachableBlock) { printf("\nAfter dead block removal:\n"); fgDispBasicBlocks(verboseTrees); @@ -732,7 +760,7 @@ bool Compiler::fgRemoveDeadBlocks() fgVerifyHandlerTab(); fgDebugCheckBBlist(false); #endif // DEBUG - return changed; + return hasUnreachableBlock; } //------------------------------------------------------------- diff --git a/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_1.cs b/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_1.cs new file mode 100644 index 0000000000000..e1c93bc68fbf7 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_1.cs @@ -0,0 +1,35 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// +// In this issue, although we were not removing an unreachable block, we were removing all the code +// inside it and as such should update the liveness information. Since we were not updating the liveness +// information for such scenarios, we were hitting an assert during register allocation. +public class Program +{ + public static ulong[] s_14; + public static uint s_34; + public static int Main() + { + var vr2 = new ulong[][]{new ulong[]{0}}; + M27(s_34, vr2); + return 100; + } + + public static void M27(uint arg4, ulong[][] arg5) + { + arg5[0][0] = arg5[0][0]; + for (int var7 = 0; var7 < 1; var7++) + { + return; + } + + try + { + s_14 = arg5[0]; + } + finally + { + arg4 = arg4; + } + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_1.csproj b/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_1.csproj new file mode 100644 index 0000000000000..57ed2079d0201 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_1.csproj @@ -0,0 +1,13 @@ + + + Exe + true + + + PdbOnly + True + + + + + \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_2.cs b/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_2.cs new file mode 100644 index 0000000000000..a741e66919981 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_2.cs @@ -0,0 +1,46 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// +// In this issue, we were not removing all the unreachable blocks and that led us to expect that +// there should be an IG label for one of the unreachable block, but we were not creating it leading +// to an assert failure. +public class _65659_2 +{ + public static bool[][,] s_2; + public static short[,][] s_8; + public static bool[] s_10; + public static ushort[][] s_29; + public static int Main() + { + bool vr1 = M47(); + return 100; + } + + public static bool M47() + { + bool var0 = default(bool); + try + { + if (var0) + { + try + { + if (s_10[0]) + { + return s_2[0][0, 0]; + } + } + finally + { + s_8[0, 0][0] = 0; + } + } + } + finally + { + s_29 = s_29; + } + + return true; + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_2.csproj b/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_2.csproj new file mode 100644 index 0000000000000..57ed2079d0201 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_2.csproj @@ -0,0 +1,13 @@ + + + Exe + true + + + PdbOnly + True + + + + + \ No newline at end of file