From 401e8222840875bd1c97de8e039cce32c222d582 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Thu, 26 May 2022 15:34:51 -0700 Subject: [PATCH 1/8] Track if unreachable blocks were detected squash: Update test case name --- src/coreclr/jit/fgopt.cpp | 19 +++++++++--- .../JitBlue/GitHub_69659/GitHub_69659_1.cs | 31 +++++++++++++++++++ .../GitHub_69659/GitHub_69659_1.csproj | 13 ++++++++ 3 files changed, 58 insertions(+), 5 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 diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 228e01cbf5ad4..a340899cb83cf 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -713,16 +713,25 @@ 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); + auto isBlockRemovable = [&](BasicBlock* block) -> bool + { + bool isVisited = BlockSetOps::IsMember(this, visitedBlocks, block->bbNum); + hasUnreachableBlock |= !isVisited; + return !isVisited; }; - bool changed = fgRemoveUnreachableBlocks(isBlockRemovable); + fgRemoveUnreachableBlocks(isBlockRemovable); #ifdef DEBUG - if (verbose && changed) + if (verbose && hasUnreachableBlock) { printf("\nAfter dead block removal:\n"); fgDispBasicBlocks(verboseTrees); @@ -732,7 +741,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..1882e89f734a6 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_1.cs @@ -0,0 +1,31 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +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 From b2319194693b45d95c4e859360efa1d6563f3776 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 27 May 2022 06:36:49 -0700 Subject: [PATCH 2/8] Unreachable one-by-one unreachable to squash --- src/coreclr/jit/fgopt.cpp | 36 +++++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index a340899cb83cf..ff09a4abda6d3 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -654,6 +654,8 @@ bool Compiler::fgRemoveDeadBlocks() JITDUMP("\n*************** In fgRemoveDeadBlocks()"); jitstd::list worklist(jitstd::allocator(getAllocator(CMK_Reachability))); + jitstd::list visitedlist(jitstd::allocator(getAllocator(CMK_Reachability))); + worklist.push_back(fgFirstBB); // Do not remove handler blocks @@ -706,6 +708,7 @@ bool Compiler::fgRemoveDeadBlocks() } BlockSetOps::AddElemD(this, visitedBlocks, block->bbNum); + visitedlist.push_back(block); for (BasicBlock* succ : block->Succs(this)) { @@ -721,14 +724,41 @@ bool Compiler::fgRemoveDeadBlocks() // 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 - { + auto isBlockRemovable = [&](BasicBlock* block) -> bool { bool isVisited = BlockSetOps::IsMember(this, visitedBlocks, block->bbNum); hasUnreachableBlock |= !isVisited; return !isVisited; }; - fgRemoveUnreachableBlocks(isBlockRemovable); + bool changed = false; + int iterationCount = 0; + do + { + changed = false; + fgRemoveUnreachableBlocks(isBlockRemovable); + + // Check if we produced some more unreachable blocks and if yes, remove them as well. + // Note: We skip recalculating all the visited blocks and instead just remove them from + // the visited list because of two reasons - We won't have to recreate the visited list + // and it is rare to have a scenario. + // + // TODO-CQ: Ideally, we should have a PriorityQueue that sorts the blocks based on bbRefs + // and remove them as they become unreachable. + for (auto reachableBlock : visitedlist) + { + if (reachableBlock->bbRefs == 0) + { + if (BlockSetOps::IsMember(this, visitedBlocks, reachableBlock->bbNum) && + ((reachableBlock->bbFlags & BBF_REMOVED) == 0)) + { + changed = true; + BlockSetOps::RemoveElemD(this, visitedBlocks, reachableBlock->bbNum); + JITDUMP("Found a reachable block " FMT_BB " that now has zero refs\n", reachableBlock->bbNum); + } + } + } + iterationCount++; + } while (changed && iterationCount < 3); #ifdef DEBUG if (verbose && hasUnreachableBlock) From 37955fe58e1fa066472c397038a330ef1aaa3a0f Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 27 May 2022 07:55:28 -0700 Subject: [PATCH 3/8] Add test cases --- .../JitBlue/GitHub_69659/GitHub_69659_2.cs | 42 +++++++++++++++++++ .../GitHub_69659/GitHub_69659_2.csproj | 13 ++++++ 2 files changed, 55 insertions(+) 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/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..d91affff13099 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_2.cs @@ -0,0 +1,42 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +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 From 2dc4c514a7097b71bda3b45f626c444ebd5ff690 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 27 May 2022 07:58:48 -0700 Subject: [PATCH 4/8] add test description --- .../JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_1.cs | 4 ++++ .../JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_2.cs | 4 ++++ 2 files changed, 8 insertions(+) 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 index 1882e89f734a6..e1c93bc68fbf7 100644 --- a/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_1.cs +++ b/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_1.cs @@ -1,5 +1,9 @@ // 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; 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 index d91affff13099..a741e66919981 100644 --- a/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_2.cs +++ b/src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_2.cs @@ -1,5 +1,9 @@ // 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; From 4d2857e9bf78bdc55bfd43f454d3ebfe0efd19ca Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Tue, 31 May 2022 06:25:00 -0700 Subject: [PATCH 5/8] Check if there were any unreachable blocks --- src/coreclr/jit/fgopt.cpp | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index ff09a4abda6d3..57177ed8fc0ae 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -735,25 +735,26 @@ bool Compiler::fgRemoveDeadBlocks() do { changed = false; - fgRemoveUnreachableBlocks(isBlockRemovable); - - // Check if we produced some more unreachable blocks and if yes, remove them as well. - // Note: We skip recalculating all the visited blocks and instead just remove them from - // the visited list because of two reasons - We won't have to recreate the visited list - // and it is rare to have a scenario. - // - // TODO-CQ: Ideally, we should have a PriorityQueue that sorts the blocks based on bbRefs - // and remove them as they become unreachable. - for (auto reachableBlock : visitedlist) + if (fgRemoveUnreachableBlocks(isBlockRemovable)) { - if (reachableBlock->bbRefs == 0) + // If we produced more unreachable blocks, remove them as well. + // Note: We skip recalculating all the visited blocks and instead just remove them from + // the visited list because of two reasons - We won't have to recreate the visited list + // and it is rare scenario . + // + // TODO-CQ: Ideally, we should have a PriorityQueue that sorts the blocks based on bbRefs + // and remove them as they become unreachable. + for (auto reachableBlock : visitedlist) { - if (BlockSetOps::IsMember(this, visitedBlocks, reachableBlock->bbNum) && - ((reachableBlock->bbFlags & BBF_REMOVED) == 0)) + if (reachableBlock->bbRefs == 0) { - changed = true; - BlockSetOps::RemoveElemD(this, visitedBlocks, reachableBlock->bbNum); - JITDUMP("Found a reachable block " FMT_BB " that now has zero refs\n", reachableBlock->bbNum); + if (BlockSetOps::IsMember(this, visitedBlocks, reachableBlock->bbNum) && + ((reachableBlock->bbFlags & BBF_REMOVED) == 0)) + { + changed = true; + BlockSetOps::RemoveElemD(this, visitedBlocks, reachableBlock->bbNum); + JITDUMP("Found a reachable block " FMT_BB " that now has zero refs\n", reachableBlock->bbNum); + } } } } From 2b1fe0864369040e68838ca438c2c7462ca674b6 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 3 Jun 2022 23:29:07 -0700 Subject: [PATCH 6/8] review feedback --- src/coreclr/jit/fgopt.cpp | 44 +++++++++++++-------------------------- 1 file changed, 14 insertions(+), 30 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 57177ed8fc0ae..b333f2506deee 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -654,7 +654,6 @@ bool Compiler::fgRemoveDeadBlocks() JITDUMP("\n*************** In fgRemoveDeadBlocks()"); jitstd::list worklist(jitstd::allocator(getAllocator(CMK_Reachability))); - jitstd::list visitedlist(jitstd::allocator(getAllocator(CMK_Reachability))); worklist.push_back(fgFirstBB); @@ -708,7 +707,6 @@ bool Compiler::fgRemoveDeadBlocks() } BlockSetOps::AddElemD(this, visitedBlocks, block->bbNum); - visitedlist.push_back(block); for (BasicBlock* succ : block->Succs(this)) { @@ -726,40 +724,26 @@ bool Compiler::fgRemoveDeadBlocks() // any of the fgFirstBB, handler, filter or BBJ_ALWAYS (Arm) blocks. auto isBlockRemovable = [&](BasicBlock* block) -> bool { bool isVisited = BlockSetOps::IsMember(this, visitedBlocks, block->bbNum); - hasUnreachableBlock |= !isVisited; - return !isVisited; + bool isRemovable = (!isVisited || block->bbRefs == 0); + + hasUnreachableBlock |= isRemovable; + + return isRemovable; }; - bool changed = false; - int iterationCount = 0; + bool changed = false; + unsigned iterationCount = 1; do { - changed = false; - if (fgRemoveUnreachableBlocks(isBlockRemovable)) + JITDUMP("\nRemoving unreachable blocks for fgRemoveDeadBlocks iteration #%u\n", iterationCount); + + // Just to be paranoid, avoid infinite loops; fall back to minopts. + if (iterationCount++ > 10) { - // If we produced more unreachable blocks, remove them as well. - // Note: We skip recalculating all the visited blocks and instead just remove them from - // the visited list because of two reasons - We won't have to recreate the visited list - // and it is rare scenario . - // - // TODO-CQ: Ideally, we should have a PriorityQueue that sorts the blocks based on bbRefs - // and remove them as they become unreachable. - for (auto reachableBlock : visitedlist) - { - if (reachableBlock->bbRefs == 0) - { - if (BlockSetOps::IsMember(this, visitedBlocks, reachableBlock->bbNum) && - ((reachableBlock->bbFlags & BBF_REMOVED) == 0)) - { - changed = true; - BlockSetOps::RemoveElemD(this, visitedBlocks, reachableBlock->bbNum); - JITDUMP("Found a reachable block " FMT_BB " that now has zero refs\n", reachableBlock->bbNum); - } - } - } + noway_assert(!"Too many unreachable block removal loops"); } - iterationCount++; - } while (changed && iterationCount < 3); + changed = fgRemoveUnreachableBlocks(isBlockRemovable); + } while (changed); #ifdef DEBUG if (verbose && hasUnreachableBlock) From 223b8e19e94b94f16a8146b6ca43caf9a41ed0fa Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 6 Jun 2022 06:12:06 -0700 Subject: [PATCH 7/8] jit format --- src/coreclr/jit/fgopt.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index b333f2506deee..228fa4b4d5403 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -723,7 +723,7 @@ bool Compiler::fgRemoveDeadBlocks() // 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 { - bool isVisited = BlockSetOps::IsMember(this, visitedBlocks, block->bbNum); + bool isVisited = BlockSetOps::IsMember(this, visitedBlocks, block->bbNum); bool isRemovable = (!isVisited || block->bbRefs == 0); hasUnreachableBlock |= isRemovable; From dbccb5141ca65ffcfde674c133d343f3190f44ca Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 6 Jun 2022 07:01:18 -0700 Subject: [PATCH 8/8] Fix a case for isBBCallAlwaysPairTail for Arm --- src/coreclr/jit/fgopt.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 228fa4b4d5403..d0d4ae647ec98 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -726,6 +726,10 @@ bool Compiler::fgRemoveDeadBlocks() 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;