From d4cb13c66135b9ba7099438a374956295bdf5ca3 Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Thu, 18 Jan 2024 23:13:13 -0800 Subject: [PATCH] Fix phase order problem with throw helper blocks Throw helper blocks are created in morph, then possibly removed if unnecessary in StackLevelSetter (under optimization). However, there was a case where StackLevelSetter removed an OVERFLOW throw helper block after optimization proved it unnecessary because of a constant zero dividend, but between StackLevelSetter and codegen, LSRA introduced a RELOAD node above the constant zero that `GenTree::CanDivOrModPossiblyOverflow()` didn't understand, thus causing it to think that overflow was possible. Codegen looked for the OVERFLOW throw helper block and couldn't find it. There are multiple fixes here, somewhat "defense in depth": - If `StackLevelSetter::SetThrowHelperBlocks()` determines a node can't throw divide-by-zero or ArithmeticException (overflow), it marks the node GTF_DIV_MOD_NO_BY_ZERO / GTF_DIV_MOD_NO_OVERFLOW, respectively. This is what morph does earlier in compilation. - `genMarkLabelsForCodegen()` does not mark throw helper blocks where `acdUsed` is false, to avoid marking deleted blocks. - More asserts are added that `acdUsed` is true when codegen goes to generate a branch to a throw helper. - `GenTree::OperExceptions` / `CanDivOrModPossiblyOverflow` are changed to skip COPY/RELOAD nodes. Fixes #96224 --- src/coreclr/jit/codegencommon.cpp | 11 ++++++++--- src/coreclr/jit/codegenloongarch64.cpp | 2 ++ src/coreclr/jit/codegenriscv64.cpp | 2 ++ src/coreclr/jit/compiler.h | 6 +++++- src/coreclr/jit/compiler.hpp | 4 ++-- src/coreclr/jit/emit.cpp | 10 +++++++++- src/coreclr/jit/flowgraph.cpp | 8 ++++---- src/coreclr/jit/gentree.cpp | 11 ++++------- src/coreclr/jit/gentree.h | 5 ++--- src/coreclr/jit/stacklevelsetter.cpp | 21 +++++++++++++++++++++ 10 files changed, 59 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index f0471d242d812c..3c55f27f44235f 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -455,10 +455,13 @@ void CodeGen::genMarkLabelsForCodegen() } // Walk all the exceptional code blocks and mark them, since they don't appear in the normal flow graph. - for (Compiler::AddCodeDsc* add = compiler->fgAddCodeList; add; add = add->acdNext) + for (Compiler::AddCodeDsc* add = compiler->fgAddCodeList; add != nullptr; add = add->acdNext) { - JITDUMP(" " FMT_BB " : throw helper block\n", add->acdDstBlk->bbNum); - add->acdDstBlk->SetFlags(BBF_HAS_LABEL); + if (add->acdUsed) + { + JITDUMP(" " FMT_BB " : throw helper block\n", add->acdDstBlk->bbNum); + add->acdDstBlk->SetFlags(BBF_HAS_LABEL); + } } for (EHblkDsc* const HBtab : EHClauses(compiler)) @@ -1521,6 +1524,7 @@ void CodeGen::genJumpToThrowHlpBlk(emitJumpKind jumpKind, SpecialCodeKind codeKi #ifdef DEBUG Compiler::AddCodeDsc* add = compiler->fgFindExcptnTarget(codeKind, compiler->bbThrowIndex(compiler->compCurBB)); + assert(add->acdUsed); assert(excpRaisingBlock == add->acdDstBlk); #if !FEATURE_FIXED_OUT_ARGS assert(add->acdStkLvlInit || isFramePointerUsed()); @@ -1533,6 +1537,7 @@ void CodeGen::genJumpToThrowHlpBlk(emitJumpKind jumpKind, SpecialCodeKind codeKi Compiler::AddCodeDsc* add = compiler->fgFindExcptnTarget(codeKind, compiler->bbThrowIndex(compiler->compCurBB)); PREFIX_ASSUME_MSG((add != nullptr), ("ERROR: failed to find exception throw block")); + assert(add->acdUsed); excpRaisingBlock = add->acdDstBlk; #if !FEATURE_FIXED_OUT_ARGS assert(add->acdStkLvlInit || isFramePointerUsed()); diff --git a/src/coreclr/jit/codegenloongarch64.cpp b/src/coreclr/jit/codegenloongarch64.cpp index 0a15bdf1dc7c89..03471bc892f51c 100644 --- a/src/coreclr/jit/codegenloongarch64.cpp +++ b/src/coreclr/jit/codegenloongarch64.cpp @@ -7671,6 +7671,7 @@ inline void CodeGen::genJumpToThrowHlpBlk_la( #ifdef DEBUG Compiler::AddCodeDsc* add = compiler->fgFindExcptnTarget(codeKind, compiler->bbThrowIndex(compiler->compCurBB)); + assert(add->acdUsed); assert(excpRaisingBlock == add->acdDstBlk); #if !FEATURE_FIXED_OUT_ARGS assert(add->acdStkLvlInit || isFramePointerUsed()); @@ -7683,6 +7684,7 @@ inline void CodeGen::genJumpToThrowHlpBlk_la( Compiler::AddCodeDsc* add = compiler->fgFindExcptnTarget(codeKind, compiler->bbThrowIndex(compiler->compCurBB)); PREFIX_ASSUME_MSG((add != nullptr), ("ERROR: failed to find exception throw block")); + assert(add->acdUsed); excpRaisingBlock = add->acdDstBlk; #if !FEATURE_FIXED_OUT_ARGS assert(add->acdStkLvlInit || isFramePointerUsed()); diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index f3bc1b10c5bdb0..3e2788c385a733 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -7186,6 +7186,7 @@ inline void CodeGen::genJumpToThrowHlpBlk_la( #ifdef DEBUG Compiler::AddCodeDsc* add = compiler->fgFindExcptnTarget(codeKind, compiler->bbThrowIndex(compiler->compCurBB)); + assert(add->acdUsed); assert(excpRaisingBlock == add->acdDstBlk); #if !FEATURE_FIXED_OUT_ARGS assert(add->acdStkLvlInit || isFramePointerUsed()); @@ -7198,6 +7199,7 @@ inline void CodeGen::genJumpToThrowHlpBlk_la( Compiler::AddCodeDsc* add = compiler->fgFindExcptnTarget(codeKind, compiler->bbThrowIndex(compiler->compCurBB)); PREFIX_ASSUME_MSG((add != nullptr), ("ERROR: failed to find exception throw block")); + assert(add->acdUsed); excpRaisingBlock = add->acdDstBlk; #if !FEATURE_FIXED_OUT_ARGS assert(add->acdStkLvlInit || isFramePointerUsed()); diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 411a14976907bf..ff949c4bc082ae 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6554,7 +6554,11 @@ class Compiler struct AddCodeDsc { AddCodeDsc* acdNext; - BasicBlock* acdDstBlk; // block to which we jump + + // Initially the source block of the exception. After fgCreateThrowHelperBlocks, the block to which + // we jump to raise the exception. + BasicBlock* acdDstBlk; + unsigned acdData; SpecialCodeKind acdKind; // what kind of a special block is this? bool acdUsed; // do we need to keep this helper block? diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 4f10b648efb549..a686de150d5fe0 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -3045,7 +3045,7 @@ inline bool Compiler::fgIsThrowHlpBlk(BasicBlock* block) // under stress, with implausible flow graph optimizations. So, walk the fgAddCodeList // for the final determination. - for (AddCodeDsc* add = fgAddCodeList; add; add = add->acdNext) + for (AddCodeDsc* add = fgAddCodeList; add != nullptr; add = add->acdNext) { if (block == add->acdDstBlk) { @@ -3068,7 +3068,7 @@ inline bool Compiler::fgIsThrowHlpBlk(BasicBlock* block) inline unsigned Compiler::fgThrowHlpBlkStkLevel(BasicBlock* block) { - for (AddCodeDsc* add = fgAddCodeList; add; add = add->acdNext) + for (AddCodeDsc* add = fgAddCodeList; add != nullptr; add = add->acdNext) { if (block == add->acdDstBlk) { diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 89b2b983f1711b..b993c29e1dda31 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -4299,7 +4299,15 @@ void emitter::emitDispJumpList() else #endif // TARGET_ARM64 { - printf(" -> IG%02u", ((insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel))->igNum); + insGroup* targetGroup = (insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel); + if (targetGroup == nullptr) + { + printf(" -> ILLEGAL"); + } + else + { + printf(" -> IG%02u", targetGroup->igNum); + } } if (jmp->idjShort) diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 7a12698b8481af..8ffcd599eced9a 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -3539,15 +3539,15 @@ PhaseStatus Compiler::fgCreateThrowHelperBlocks() } //------------------------------------------------------------------------ -// fgFindExcptnTarget: finds the block to jump that will throw a given kind of exception +// fgFindExcptnTarget: finds the block to jump to that will throw a given kind of exception // // Arguments: -// kind - kind of exception to throw +// kind -- kind of exception to throw // refData -- bbThrowIndex of the block that will jump to the throw helper // // Return Value: // Code descriptor for the appropriate throw helper block, or nullptr if no such -// descriptor exists +// descriptor exists. // Compiler::AddCodeDsc* Compiler::fgFindExcptnTarget(SpecialCodeKind kind, unsigned refData) { @@ -3559,7 +3559,7 @@ Compiler::AddCodeDsc* Compiler::fgFindExcptnTarget(SpecialCodeKind kind, unsigne if (add == nullptr) { - // We should't be asking for these blocks late in compilation + // We shouldn't be asking for these blocks late in compilation // unless we know there are entries to be found. assert(!fgRngChkThrowAdded); } diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 0ad8c48afc106b..ac88ed8abae8b9 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6908,7 +6908,6 @@ bool GenTree::OperIsImplicitIndir() const //------------------------------------------------------------------------------ // OperExceptions: Get exception set this tree may throw. // -// // Arguments: // comp - Compiler instance // @@ -6945,11 +6944,9 @@ ExceptionSetFlags GenTree::OperExceptions(Compiler* comp) ExceptionSetFlags exSetFlags = ExceptionSetFlags::None; - GenTree* op2 = this->gtGetOp2(); - - if (!(this->gtFlags & GTF_DIV_MOD_NO_BY_ZERO) && !op2->IsNeverZero()) + if (!(this->gtFlags & GTF_DIV_MOD_NO_BY_ZERO) && !this->gtGetOp2()->gtSkipReloadOrCopy()->IsNeverZero()) { - exSetFlags = ExceptionSetFlags::DivideByZeroException; + exSetFlags |= ExceptionSetFlags::DivideByZeroException; } if (this->OperIs(GT_DIV, GT_MOD) && this->CanDivOrModPossiblyOverflow(comp)) @@ -27217,8 +27214,8 @@ bool GenTree::CanDivOrModPossiblyOverflow(Compiler* comp) const if (this->gtFlags & GTF_DIV_MOD_NO_OVERFLOW) return false; - GenTree* op1 = this->gtGetOp1(); - GenTree* op2 = this->gtGetOp2(); + GenTree* op1 = this->gtGetOp1()->gtSkipReloadOrCopy(); + GenTree* op2 = this->gtGetOp2()->gtSkipReloadOrCopy(); // If the divisor is known to never be '-1', we cannot overflow. if (op2->IsNeverNegativeOne(comp)) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 0902c00c1650c1..4eaadb26e76294 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -8910,7 +8910,6 @@ inline bool GenTree::OperIsCopyBlkOp() // long constants in a target-independent way. inline bool GenTree::IsIntegralConst(ssize_t constVal) const - { if ((gtOper == GT_CNS_INT) && (AsIntConCommon()->IconValue() == constVal)) { @@ -9328,9 +9327,9 @@ inline GenTree* GenTree::gtCommaStoreVal() inline GenTree* GenTree::gtSkipReloadOrCopy() { // There can be only one reload or copy (we can't have a reload/copy of a reload/copy) - if (gtOper == GT_RELOAD || gtOper == GT_COPY) + if (OperIs(GT_RELOAD, GT_COPY)) { - assert(gtGetOp1()->OperGet() != GT_RELOAD && gtGetOp1()->OperGet() != GT_COPY); + assert(!gtGetOp1()->OperIs(GT_RELOAD, GT_COPY)); return gtGetOp1(); } return this; diff --git a/src/coreclr/jit/stacklevelsetter.cpp b/src/coreclr/jit/stacklevelsetter.cpp index 4f37e1621e6fd5..d422b6559632cb 100644 --- a/src/coreclr/jit/stacklevelsetter.cpp +++ b/src/coreclr/jit/stacklevelsetter.cpp @@ -80,6 +80,14 @@ PhaseStatus StackLevelSetter::DoPhase() madeChanges = true; } } + else + { + // Mark all the throw helpers as used to avoid asserts later. + for (Compiler::AddCodeDsc* add = comp->fgGetAdditionalCodeDescriptors(); add != nullptr; add = add->acdNext) + { + add->acdUsed = true; + } + } // The above loop might have moved a BBJ_COND's true target to its next block. // In such cases, reverse the condition so we can remove a branch. @@ -190,6 +198,7 @@ void StackLevelSetter::ProcessBlock(BasicBlock* block) // Arguments: // node - the node to process; // block - the source block for the node. +// void StackLevelSetter::SetThrowHelperBlocks(GenTree* node, BasicBlock* block) { assert(node->OperMayThrow(comp)); @@ -227,11 +236,21 @@ void StackLevelSetter::SetThrowHelperBlocks(GenTree* node, BasicBlock* block) { SetThrowHelperBlock(SCK_DIV_BY_ZERO, block); } + else + { + // Even if we thought it might divide by zero during morph, now we know it never will. + node->gtFlags |= GTF_DIV_MOD_NO_BY_ZERO; + } if ((exSetFlags & ExceptionSetFlags::ArithmeticException) != ExceptionSetFlags::None) { SetThrowHelperBlock(SCK_ARITH_EXCPN, block); } + else + { + // Even if we thought it might overflow during morph, now we know it never will. + node->gtFlags |= GTF_DIV_MOD_NO_OVERFLOW; + } } break; #endif @@ -256,10 +275,12 @@ void StackLevelSetter::SetThrowHelperBlocks(GenTree* node, BasicBlock* block) // Arguments: // kind - the special throw-helper kind; // block - the source block that targets helper. +// void StackLevelSetter::SetThrowHelperBlock(SpecialCodeKind kind, BasicBlock* block) { Compiler::AddCodeDsc* add = comp->fgFindExcptnTarget(kind, comp->bbThrowIndex(block)); assert(add != nullptr); + // We expect we'll actually need this helper. add->acdUsed = true;