From 41953b7cb36c748470a2125da85005a54299a48e Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Thu, 12 Oct 2023 16:06:10 -0700 Subject: [PATCH 1/3] Improve EH dead code elimination Update `fgRemoveDeadBlocks()` to only treat EH handlers as reachable if their corresponding `try` block entry is reachable. This causes more dead code to be eliminated in handlers that are not reachable. There is a little more work to do to completely clean these up: 1. If a `try` is not reachable, we should get rid of its entire EH table entry. Currently, its block is generated as `int3` (on xarch). 2. Even though the handler is not reachable, we still generate the prolog followed by `int3` for the body. Contributes to #82335 --- src/coreclr/jit/fgopt.cpp | 77 +++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 35 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 57abb39ac55b7c..76d1d5ccf8bb01 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -631,34 +631,6 @@ bool Compiler::fgRemoveDeadBlocks() { JITDUMP("\n*************** In fgRemoveDeadBlocks()"); - jitstd::list worklist(jitstd::allocator(getAllocator(CMK_Reachability))); - - worklist.push_back(fgFirstBB); - - // Do not remove handler blocks - for (EHblkDsc* const HBtab : EHClauses(this)) - { - if (HBtab->HasFilter()) - { - worklist.push_back(HBtab->ebdFilter); - } - worklist.push_back(HBtab->ebdHndBeg); - } - -#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) - // For ARM code, prevent creating retless calls by adding the BBJ_ALWAYS to the "fgAlwaysBlks" list. - for (BasicBlock* const block : Blocks()) - { - if (block->KindIs(BBJ_CALLFINALLY)) - { - assert(block->isBBCallAlwaysPair()); - - // Don't remove the BBJ_ALWAYS block that is only here for the unwinder. - worklist.push_back(block->Next()); - } - } -#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) - unsigned prevFgCurBBEpoch = fgCurBBEpoch; EnsureBasicBlockEpoch(); @@ -673,6 +645,9 @@ bool Compiler::fgRemoveDeadBlocks() BlockSet visitedBlocks(BlockSetOps::MakeEmpty(this)); + jitstd::list worklist(jitstd::allocator(getAllocator(CMK_Reachability))); + worklist.push_back(fgFirstBB); + // Visit all the reachable blocks, everything else can be removed while (!worklist.empty()) { @@ -690,6 +665,43 @@ bool Compiler::fgRemoveDeadBlocks() { worklist.push_back(succ); } + + // Add all the "EH" successors. For every `try`, add its handler (including filter) to the worklist. + if (bbIsTryBeg(block)) + { + // Due to EH normalization, a block can only be the start of a single `try` region, with the exception + // of mutually-protect regions. + assert(block->hasTryIndex()); + unsigned tryIndex = block->getTryIndex(); + EHblkDsc* ehDsc = ehGetDsc(tryIndex); + for (;;) + { + worklist.push_back(ehDsc->ebdHndBeg); + if (ehDsc->HasFilter()) + { + worklist.push_back(ehDsc->ebdFilter); + } + tryIndex = ehDsc->ebdEnclosingTryIndex; + if (tryIndex == EHblkDsc::NO_ENCLOSING_INDEX) + { + break; + } + ehDsc = ehGetDsc(tryIndex); + if (ehDsc->ebdTryBeg != block) + { + break; + } + } + } + +#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + // For ARM code, always keep BBJ_CALLFINALLY/BBJ_ALWAYS as a pair + if (block->KindIs(BBJ_CALLFINALLY)) + { + assert(block->isBBCallAlwaysPair()); + worklist.push_back(block->Next()); + } +#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) } // Track if there is any unreachable block. Even if it is marked with @@ -702,14 +714,8 @@ 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); - 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 + bool isRemovable = !isVisited || (block->bbRefs == 0); hasUnreachableBlock |= isRemovable; - return isRemovable; }; @@ -738,6 +744,7 @@ bool Compiler::fgRemoveDeadBlocks() fgVerifyHandlerTab(); fgDebugCheckBBlist(false); #endif // DEBUG + return hasUnreachableBlock; } From 335e2d31f9b70371ff71d6066b09ad283e419eba Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Fri, 13 Oct 2023 12:26:08 -0700 Subject: [PATCH 2/3] Fix BBJ_EHFILTERRET handling in fgRemoveBlockAsPred fgRemoveBlockAsPred had special handling to not decrease the bbRefs count of the filter-handler targeted by BBJ_EHFILTERRET. This is incorrect, as the filter-handler already has an extra "beginning of handler" extra ref count. Possibly this code was never invoked before as filters were not previously removed. Also, fix the JitDump output of the filter ret block in block dumps. --- src/coreclr/jit/block.cpp | 2 +- src/coreclr/jit/clrjit.natvis | 2 +- src/coreclr/jit/fgbasic.cpp | 2 +- src/coreclr/jit/fgdiagnostic.cpp | 3 ++- src/coreclr/jit/fgflow.cpp | 6 +----- src/coreclr/jit/fgopt.cpp | 7 +++++++ 6 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 96267961ed1e9f..31e16f084cf31a 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -637,7 +637,7 @@ void BasicBlock::dspJumpKind() break; case BBJ_EHFILTERRET: - printf(" (fltret)"); + printf(" -> " FMT_BB " (fltret)", bbJumpDest->bbNum); break; case BBJ_EHCATCHRET: diff --git a/src/coreclr/jit/clrjit.natvis b/src/coreclr/jit/clrjit.natvis index fb0f2c42dafce5..f56ac98cf6a168 100644 --- a/src/coreclr/jit/clrjit.natvis +++ b/src/coreclr/jit/clrjit.natvis @@ -21,7 +21,7 @@ Documentation for VS debugger format specifiers: https://docs.microsoft.com/en-u - BB{bbNum,d}->BB{bbJumpDest->bbNum,d}; {bbJumpKind,en} + BB{bbNum,d}->BB{bbJumpDest->bbNum,d}; {bbJumpKind,en} BB{bbNum,d}; {bbJumpKind,en}; {bbJumpSwt->bbsCount} cases BB{bbNum,d}; {bbJumpKind,en}; {bbJumpEhf->bbeCount} succs BB{bbNum,d}; {bbJumpKind,en} diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 7b4b4dda596ec8..63bd1f1ff3d662 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -268,7 +268,7 @@ bool Compiler::fgEnsureFirstBBisScratch() } // The first block has an implicit ref count which we must - // remove. Note the ref count could be greater that one, if + // remove. Note the ref count could be greater than one, if // the first block is not scratch and is targeted by a // branch. assert(fgFirstBB->bbRefs >= 1); diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 993d0af4e86cc9..4f5ed77a1bc189 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -2077,7 +2077,8 @@ void Compiler::fgTableDispBasicBlock(BasicBlock* block, int ibcColWidth /* = 0 * break; case BBJ_EHFILTERRET: - printf("%*s (fltret)", maxBlockNumWidth - 2, ""); + printf("-> " FMT_BB "%*s (fltret)", block->GetJumpDest()->bbNum, + maxBlockNumWidth - max(CountDigits(block->GetJumpDest()->bbNum), 2), ""); break; case BBJ_EHCATCHRET: diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index f5a35017371a59..363ad7c2a17dc8 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -346,6 +346,7 @@ void Compiler::fgRemoveBlockAsPred(BasicBlock* block) case BBJ_CALLFINALLY: case BBJ_ALWAYS: case BBJ_EHCATCHRET: + case BBJ_EHFILTERRET: fgRemoveRefPred(block->GetJumpDest(), block); break; @@ -358,11 +359,6 @@ void Compiler::fgRemoveBlockAsPred(BasicBlock* block) fgRemoveRefPred(block->Next(), block); break; - case BBJ_EHFILTERRET: - block->GetJumpDest()->bbRefs++; // To compensate the bbRefs-- inside fgRemoveRefPred - fgRemoveRefPred(block->GetJumpDest(), block); - break; - case BBJ_EHFINALLYRET: for (BasicBlock* const succ : block->EHFinallyRetSuccs()) { diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 76d1d5ccf8bb01..0707871df1109c 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -437,6 +437,11 @@ bool Compiler::fgRemoveUnreachableBlocks(CanRemoveBlockBody canRemoveBlock) // to properly set the info.compProfilerCallback flag. continue; } + else if ((block->bbFlags & BBF_DONT_REMOVE) && block->isEmpty() && block->KindIs(BBJ_THROW)) + { + // We already converted a non-removable block to a throw; don't bother processing it again. + continue; + } else if (!canRemoveBlock(block)) { continue; @@ -458,6 +463,8 @@ bool Compiler::fgRemoveUnreachableBlocks(CanRemoveBlockBody canRemoveBlock) // Unmark the block as removed, clear BBF_INTERNAL, and set BBJ_IMPORTED + JITDUMP("Converting BBF_DONT_REMOVE block " FMT_BB " to BBJ_THROW\n", block->bbNum); + // The successors may be unreachable after this change. changed |= block->NumSucc() > 0; From 430f171f2f834d5e2307178993e0e9ee6f425d9a Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Fri, 13 Oct 2023 16:59:19 -0700 Subject: [PATCH 3/3] Put back arm32 BBJ_CALLFINALLY/BBJ_ALWAYS pair check --- src/coreclr/jit/fgopt.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 0707871df1109c..393924f8ec07eb 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -722,6 +722,12 @@ bool Compiler::fgRemoveDeadBlocks() auto isBlockRemovable = [&](BasicBlock* block) -> bool { bool isVisited = BlockSetOps::IsMember(this, visitedBlocks, block->bbNum); bool isRemovable = !isVisited || (block->bbRefs == 0); + +#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + // Can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair, even if bbRefs == 0. + isRemovable &= !block->isBBCallAlwaysPairTail(); +#endif + hasUnreachableBlock |= isRemovable; return isRemovable; };