From a5d6a3f07b2d54a7123ece34df752b2c8e56a49e Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 13 Nov 2023 14:57:05 -0800 Subject: [PATCH] JIT: consistently handle no return calls in qmarks When we expand QMARKS, ensure that any block with a no-return call gets changed to BBJ_THROW. This fixes a case I am seeing with cross-block local assertion prop, as the upper QMARK gets optimized away and so we don't check if the expansing has any noreturn calls. It also happens in places with just within-block local assertion prop. Contributes to #94363. --- src/coreclr/jit/compiler.h | 4 +- src/coreclr/jit/importer.cpp | 2 + src/coreclr/jit/morph.cpp | 281 +++++++++++++++++++++-------------- 3 files changed, 176 insertions(+), 111 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 48a3278077f622..8da80a7e2ee0ac 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4892,8 +4892,8 @@ class Compiler Statement* fgNewStmtFromTree(GenTree* tree, const DebugInfo& di); GenTree* fgGetTopLevelQmark(GenTree* expr, GenTree** ppDst = nullptr); - void fgExpandQmarkForCastInstOf(BasicBlock* block, Statement* stmt); - void fgExpandQmarkStmt(BasicBlock* block, Statement* stmt); + bool fgExpandQmarkForCastInstOf(BasicBlock* block, Statement* stmt); + bool fgExpandQmarkStmt(BasicBlock* block, Statement* stmt); void fgExpandQmarkNodes(); bool fgSimpleLowerCastOfSmpOp(LIR::Range& range, GenTreeCast* cast); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index d77fcfb5ef02b5..034c0d00290097 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -5652,6 +5652,8 @@ GenTree* Compiler::impCastClassOrIsInstToTree( { // condTrue is used only for throwing InvalidCastException in case of casting to an exact class. condTrue->AsCall()->gtCallMoreFlags |= GTF_CALL_M_DOES_NOT_RETURN; + + // defer calling setMethodHasNoReturnCalls until qmark expasion } } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 8460ef144f2abd..58c74e955f91f5 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14447,37 +14447,46 @@ GenTree* Compiler::fgGetTopLevelQmark(GenTree* expr, GenTree** ppDst /* = NULL * return topQmark; } -/********************************************************************************* - * - * For a castclass helper call, - * Importer creates the following tree: - * tmp = (op1 == null) ? op1 : ((*op1 == (cse = op2, cse)) ? op1 : helper()); - * - * This method splits the qmark expression created by the importer into the - * following blocks: (block, asg, cond1, cond2, helper, remainder) - * Notice that op1 is the result for both the conditions. So we coalesce these - * assignments into a single block instead of two blocks resulting a nested diamond. - * - * +---------->-----------+ - * | | | - * ^ ^ v - * | | | - * block-->asg-->cond1--+-->cond2--+-->helper--+-->remainder - * - * We expect to achieve the following codegen: - * mov rsi, rdx tmp = op1 // asgBlock - * test rsi, rsi goto skip if tmp == null ? // cond1Block - * je SKIP - * mov rcx, 0x76543210 cns = op2 // cond2Block - * cmp qword ptr [rsi], rcx goto skip if *tmp == op2 - * je SKIP - * call CORINFO_HELP_CHKCASTCLASS_SPECIAL tmp = helper(cns, tmp) // helperBlock - * mov rsi, rax - * SKIP: // remainderBlock - * tmp has the result. - * - */ -void Compiler::fgExpandQmarkForCastInstOf(BasicBlock* block, Statement* stmt) +//------------------------------------------------------------------------ +// fgExpandQmarkForCastInstOf: expand qmark for cast +// +// Arguments: +// block - block containing the qmark +// stmt - statement containing the qmark +// +// Returns: +// true if the expansion introduced a throwing block +// +// Notes: +// +// For a castclass helper call, +// Importer creates the following tree: +// tmp = (op1 == null) ? op1 : ((*op1 == (cse = op2, cse)) ? op1 : helper()); +// +// This method splits the qmark expression created by the importer into the +// following blocks: (block, asg, cond1, cond2, helper, remainder) +// Notice that op1 is the result for both the conditions. So we coalesce these +// assignments into a single block instead of two blocks resulting a nested diamond. +// +// +---------->-----------+ +// | | | +// ^ ^ v +// | | | +// block-->asg-->cond1--+-->cond2--+-->helper--+-->remainder +// +// We expect to achieve the following codegen: +// mov rsi, rdx tmp = op1 // asgBlock +// test rsi, rsi goto skip if tmp == null ? // cond1Block +// je SKIP +// mov rcx, 0x76543210 cns = op2 // cond2Block +// cmp qword ptr [rsi], rcx goto skip if *tmp == op2 +// je SKIP +// call CORINFO_HELP_CHKCASTCLASS_SPECIAL tmp = helper(cns, tmp) // helperBlock +// mov rsi, rax +// SKIP: // remainderBlock +// tmp has the result. +// +bool Compiler::fgExpandQmarkForCastInstOf(BasicBlock* block, Statement* stmt) { #ifdef DEBUG if (verbose) @@ -14487,7 +14496,8 @@ void Compiler::fgExpandQmarkForCastInstOf(BasicBlock* block, Statement* stmt) } #endif // DEBUG - GenTree* expr = stmt->GetRootNode(); + bool introducedThrow = false; + GenTree* expr = stmt->GetRootNode(); GenTree* dst = nullptr; GenTree* qmark = fgGetTopLevelQmark(expr, &dst); @@ -14600,21 +14610,28 @@ void Compiler::fgExpandQmarkForCastInstOf(BasicBlock* block, Statement* stmt) // Since we are adding helper in the JTRUE false path, reverse the cond2 and add the helper. gtReverseCond(cond2Expr); - GenTree* helperExprStore = - dst->OperIs(GT_STORE_LCL_FLD) - ? gtNewStoreLclFldNode(dstLclNum, dst->TypeGet(), dst->AsLclFld()->GetLclOffs(), true2Expr) - : gtNewStoreLclVarNode(dstLclNum, true2Expr)->AsLclVarCommon(); - Statement* helperStmt = fgNewStmtFromTree(helperExprStore, stmt->GetDebugInfo()); - fgInsertStmtAtEnd(helperBlock, helperStmt); - - // Finally remove the nested qmark stmt. - fgRemoveStmt(block, stmt); if (true2Expr->OperIs(GT_CALL) && (true2Expr->AsCall()->gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN)) { + Statement* helperStmt = fgNewStmtFromTree(true2Expr, stmt->GetDebugInfo()); + fgInsertStmtAtEnd(helperBlock, helperStmt); fgConvertBBToThrowBB(helperBlock); + setMethodHasNoReturnCalls(); + introducedThrow = true; + } + else + { + GenTree* helperExprStore = + dst->OperIs(GT_STORE_LCL_FLD) + ? gtNewStoreLclFldNode(dstLclNum, dst->TypeGet(), dst->AsLclFld()->GetLclOffs(), true2Expr) + : gtNewStoreLclVarNode(dstLclNum, true2Expr)->AsLclVarCommon(); + Statement* helperStmt = fgNewStmtFromTree(helperExprStore, stmt->GetDebugInfo()); + fgInsertStmtAtEnd(helperBlock, helperStmt); } + // Finally remove the nested qmark stmt. + fgRemoveStmt(block, stmt); + #ifdef DEBUG if (verbose) { @@ -14622,74 +14639,86 @@ void Compiler::fgExpandQmarkForCastInstOf(BasicBlock* block, Statement* stmt) fgDispBasicBlocks(block, remainderBlock, true); } #endif // DEBUG + + return introducedThrow; } -/***************************************************************************** - * - * Expand a statement with a top level qmark node. There are three cases, based - * on whether the qmark has both "true" and "false" arms, or just one of them. - * - * S0; - * C ? T : F; - * S1; - * - * Generates ===> - * - * bbj_always - * +---->------+ - * false | | - * S0 -->-- ~C -->-- T F -->-- S1 - * | | - * +--->--------+ - * bbj_cond(true) - * - * ----------------------------------------- - * - * S0; - * C ? T : NOP; - * S1; - * - * Generates ===> - * - * false - * S0 -->-- ~C -->-- T -->-- S1 - * | | - * +-->-------------+ - * bbj_cond(true) - * - * ----------------------------------------- - * - * S0; - * C ? NOP : F; - * S1; - * - * Generates ===> - * - * false - * S0 -->-- C -->-- F -->-- S1 - * | | - * +-->------------+ - * bbj_cond(true) - * - * If the qmark assigns to a variable, then create tmps for "then" - * and "else" results and assign the temp to the variable as a writeback step. - */ -void Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) +//------------------------------------------------------------------------ +// fgExpandQmarkStmt: expand a qmark into control flow +// +// Arguments: +// block - block containing the qmark +// stmt - statement containing the qmark +// +// Returns: +// true if the expansion introduced a throwing block +// +// Notes: +// +// Expand a statement with a top level qmark node. There are three cases, based +// on whether the qmark has both "true" and "false" arms, or just one of them. +// +// S0; +// C ? T : F; +// S1; +// +// Generates ===> +// +// bbj_always +// +---->------+ +// false | | +// S0 -->-- ~C -->-- T F -->-- S1 +// | | +// +--->--------+ +// bbj_cond(true) +// +// ----------------------------------------- +// +// S0; +// C ? T : NOP; +// S1; +// +// Generates ===> +// +// false +// S0 -->-- ~C -->-- T -->-- S1 +// | | +// +-->-------------+ +// bbj_cond(true) +// +// ----------------------------------------- +// +// S0; +// C ? NOP : F; +// S1; +// +// Generates ===> +// +// false +// S0 -->-- C -->-- F -->-- S1 +// | | +// +-->------------+ +// bbj_cond(true) +// +// If the qmark assigns to a variable, then create tmps for "then" +// and "else" results and assign the temp to the variable as a writeback step. +// +bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) { - GenTree* expr = stmt->GetRootNode(); + bool introducedThrow = false; + GenTree* expr = stmt->GetRootNode(); // Retrieve the Qmark node to be expanded. GenTree* dst = nullptr; GenTree* qmark = fgGetTopLevelQmark(expr, &dst); if (qmark == nullptr) { - return; + return false; } if (qmark->gtFlags & GTF_QMARK_CAST_INSTOF) { - fgExpandQmarkForCastInstOf(block, stmt); - return; + return fgExpandQmarkForCastInstOf(block, stmt); } #ifdef DEBUG @@ -14832,27 +14861,50 @@ void Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) if (hasTrueExpr) { - if (dst != nullptr) + if (trueExpr->OperIs(GT_CALL) && (trueExpr->AsCall()->gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN)) + { + Statement* trueStmt = fgNewStmtFromTree(trueExpr, stmt->GetDebugInfo()); + fgInsertStmtAtEnd(thenBlock, trueStmt); + fgConvertBBToThrowBB(thenBlock); + setMethodHasNoReturnCalls(); + introducedThrow = true; + } + else { - trueExpr = dst->OperIs(GT_STORE_LCL_FLD) - ? gtNewStoreLclFldNode(dstLclNum, dst->TypeGet(), dst->AsLclFld()->GetLclOffs(), trueExpr) - : gtNewStoreLclVarNode(dstLclNum, trueExpr)->AsLclVarCommon(); + if (dst != nullptr) + { + trueExpr = dst->OperIs(GT_STORE_LCL_FLD) ? gtNewStoreLclFldNode(dstLclNum, dst->TypeGet(), + dst->AsLclFld()->GetLclOffs(), trueExpr) + : gtNewStoreLclVarNode(dstLclNum, trueExpr)->AsLclVarCommon(); + } + Statement* trueStmt = fgNewStmtFromTree(trueExpr, stmt->GetDebugInfo()); + fgInsertStmtAtEnd(thenBlock, trueStmt); } - Statement* trueStmt = fgNewStmtFromTree(trueExpr, stmt->GetDebugInfo()); - fgInsertStmtAtEnd(thenBlock, trueStmt); } // Assign the falseExpr into the dst or tmp, insert in elseBlock if (hasFalseExpr) { - if (dst != nullptr) + if (falseExpr->OperIs(GT_CALL) && (falseExpr->AsCall()->gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN)) + { + Statement* falseStmt = fgNewStmtFromTree(falseExpr, stmt->GetDebugInfo()); + fgInsertStmtAtEnd(elseBlock, falseStmt); + fgConvertBBToThrowBB(elseBlock); + setMethodHasNoReturnCalls(); + introducedThrow = true; + } + else { - falseExpr = dst->OperIs(GT_STORE_LCL_FLD) - ? gtNewStoreLclFldNode(dstLclNum, dst->TypeGet(), dst->AsLclFld()->GetLclOffs(), falseExpr) - : gtNewStoreLclVarNode(dstLclNum, falseExpr)->AsLclVarCommon(); + if (dst != nullptr) + { + falseExpr = + dst->OperIs(GT_STORE_LCL_FLD) + ? gtNewStoreLclFldNode(dstLclNum, dst->TypeGet(), dst->AsLclFld()->GetLclOffs(), falseExpr) + : gtNewStoreLclVarNode(dstLclNum, falseExpr)->AsLclVarCommon(); + } + Statement* falseStmt = fgNewStmtFromTree(falseExpr, stmt->GetDebugInfo()); + fgInsertStmtAtEnd(elseBlock, falseStmt); } - Statement* falseStmt = fgNewStmtFromTree(falseExpr, stmt->GetDebugInfo()); - fgInsertStmtAtEnd(elseBlock, falseStmt); } #ifdef DEBUG @@ -14862,6 +14914,8 @@ void Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) fgDispBasicBlocks(block, remainderBlock, true); } #endif // DEBUG + + return introducedThrow; } /***************************************************************************** @@ -14872,6 +14926,8 @@ void Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) void Compiler::fgExpandQmarkNodes() { + bool introducedThrows = false; + if (compQmarkUsed) { for (BasicBlock* const block : Blocks()) @@ -14882,7 +14938,7 @@ void Compiler::fgExpandQmarkNodes() #ifdef DEBUG fgPreExpandQmarkChecks(expr); #endif - fgExpandQmarkStmt(block, stmt); + introducedThrows |= fgExpandQmarkStmt(block, stmt); } } #ifdef DEBUG @@ -14890,6 +14946,13 @@ void Compiler::fgExpandQmarkNodes() #endif } compQmarkRationalized = true; + + // TODO: if qmark expansion created throw blocks, try and merge them + // + if (introducedThrows) + { + JITDUMP("Qmark expansion created new throw blocks\n"); + } } //------------------------------------------------------------------------