diff --git a/src/coreclr/src/jit/compiler.cpp b/src/coreclr/src/jit/compiler.cpp index 6eacc9eff52fa0..1731dc4c7ada61 100644 --- a/src/coreclr/src/jit/compiler.cpp +++ b/src/coreclr/src/jit/compiler.cpp @@ -4622,23 +4622,16 @@ void Compiler::compCompile(void** methodCodePtr, ULONG* methodCodeSize, JitFlags } EndPhase(PHASE_GS_COOKIE); - /* Compute bbNum, bbRefs and bbPreds */ - - JITDUMP("\nRenumbering the basic blocks for fgComputePred\n"); + // GC Poll marking assumes block bbnums match lexical block order, + // so make sure this is the case. fgRenumberBlocks(); - noway_assert(!fgComputePredsDone); // This is the first time full (not cheap) preds will be computed. - fgComputePreds(); - EndPhase(PHASE_COMPUTE_PREDS); - /* If we need to emit GC Poll calls, mark the blocks that need them now. This is conservative and can * be optimized later. */ + fgMarkGCPollBlocks(); EndPhase(PHASE_MARK_GC_POLL_BLOCKS); - /* From this point on the flowgraph information such as bbNum, - * bbRefs or bbPreds has to be kept updated */ - // Compute the block and edge weights fgComputeBlockAndEdgeWeights(); EndPhase(PHASE_COMPUTE_EDGE_WEIGHTS); diff --git a/src/coreclr/src/jit/compiler.hpp b/src/coreclr/src/jit/compiler.hpp index eb73160b9ae746..457a16b4eae443 100644 --- a/src/coreclr/src/jit/compiler.hpp +++ b/src/coreclr/src/jit/compiler.hpp @@ -2729,13 +2729,31 @@ inline unsigned Compiler::fgThrowHlpBlkStkLevel(BasicBlock* block) */ inline void Compiler::fgConvertBBToThrowBB(BasicBlock* block) { - // If we're converting a BBJ_CALLFINALLY block to a BBJ_THROW block, + JITDUMP("Converting " FMT_BB " to BBJ_THROW\n", block->bbNum); + + // Ordering of the following operations matters. + // First, note if we are looking at the first block of a call always pair. + const bool isCallAlwaysPair = block->isBBCallAlwaysPair(); + + // Scrub this block from the pred lists of any successors + fgRemoveBlockAsPred(block); + + // Update jump kind after the scrub. + block->bbJumpKind = BBJ_THROW; + + // Any block with a throw is rare + block->bbSetRunRarely(); + + // If we've converted a BBJ_CALLFINALLY block to a BBJ_THROW block, // then mark the subsequent BBJ_ALWAYS block as unreferenced. - if (block->isBBCallAlwaysPair()) + // + // Must do this after we update bbJumpKind of block. + if (isCallAlwaysPair) { BasicBlock* leaveBlk = block->bbNext; noway_assert(leaveBlk->bbJumpKind == BBJ_ALWAYS); + // leaveBlk is now unreachable, so scrub the pred lists. leaveBlk->bbFlags &= ~BBF_DONT_REMOVE; leaveBlk->bbRefs = 0; leaveBlk->bbPreds = nullptr; @@ -2757,9 +2775,6 @@ inline void Compiler::fgConvertBBToThrowBB(BasicBlock* block) } #endif // defined(FEATURE_EH_FUNCLETS) && defined(_TARGET_ARM_) } - - block->bbJumpKind = BBJ_THROW; - block->bbSetRunRarely(); // any block with a throw is rare } /***************************************************************************** diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp index 080e2d5cdf330b..d4be3d96414dd0 100644 --- a/src/coreclr/src/jit/flowgraph.cpp +++ b/src/coreclr/src/jit/flowgraph.cpp @@ -419,23 +419,31 @@ BasicBlock* Compiler::fgNewBasicBlock(BBjumpKinds jumpKind) return block; } -/***************************************************************************** - * - * Ensures that fgFirstBB is a scratch BasicBlock that we have added. - * This can be used to add initialization code (without worrying - * about other blocks jumping to it). - * - * Callers have to be careful that they do not mess up the order of things - * added to fgEnsureFirstBBisScratch in a way as to change semantics. - */ - +//------------------------------------------------------------------------ +// fgEnsureFirstBBisScratch: Ensure that fgFirstBB is a scratch BasicBlock +// +// Returns: +// Nothing. May allocate a new block and alter the value of fgFirstBB. +// +// Notes: +// This should be called before adding on-entry initialization code to +// the method, to ensure that fgFirstBB is not part of a loop. +// +// Does nothing, if fgFirstBB is already a scratch BB. After calling this, +// fgFirstBB may already contain code. Callers have to be careful +// that they do not mess up the order of things added to this block and +// inadvertently change semantics. +// +// We maintain the invariant that a scratch BB ends with BBJ_NONE or +// BBJ_ALWAYS, so that when adding independent bits of initialization, +// callers can generally append to the fgFirstBB block without worring +// about what code is there already. +// +// Can be called at any time, and can be called multiple times. +// void Compiler::fgEnsureFirstBBisScratch() { - // This method does not update predecessor lists and so must only be called before they are computed. - assert(!fgComputePredsDone); - // Have we already allocated a scratch block? - if (fgFirstBBisScratch()) { return; @@ -453,6 +461,15 @@ void Compiler::fgEnsureFirstBBisScratch() block->inheritWeight(fgFirstBB); } + // The first block has an implicit ref count which we must + // remove. Note the ref count could be greater that one, if + // the first block is not scratch and is targeted by a + // branch. + assert(fgFirstBB->bbRefs >= 1); + fgFirstBB->bbRefs--; + + // The new scratch bb will fall through to the old first bb + fgAddRefPred(fgFirstBB, block); fgInsertBBbefore(fgFirstBB, block); } else @@ -466,6 +483,9 @@ void Compiler::fgEnsureFirstBBisScratch() block->bbFlags |= (BBF_INTERNAL | BBF_IMPORTED); + // This new first BB has an implicit ref, and no others. + block->bbRefs = 1; + fgFirstBBScratch = fgFirstBB; #ifdef DEBUG @@ -476,6 +496,12 @@ void Compiler::fgEnsureFirstBBisScratch() #endif } +//------------------------------------------------------------------------ +// fgFirstBBisScratch: Check if fgFirstBB is a scratch block +// +// Returns: +// true if fgFirstBB is a scratch block. +// bool Compiler::fgFirstBBisScratch() { if (fgFirstBBScratch != nullptr) @@ -497,6 +523,15 @@ bool Compiler::fgFirstBBisScratch() } } +//------------------------------------------------------------------------ +// fgBBisScratch: Check if a given block is a scratch block. +// +// Arguments: +// block - block in question +// +// Returns: +// true if this block is the first block and is a scratch block. +// bool Compiler::fgBBisScratch(BasicBlock* block) { return fgFirstBBisScratch() && (block == fgFirstBB); @@ -8244,7 +8279,7 @@ void Compiler::fgConvertSyncReturnToLeave(BasicBlock* block) // Convert the BBJ_RETURN to BBJ_ALWAYS, jumping to genReturnBB. block->bbJumpKind = BBJ_ALWAYS; block->bbJumpDest = genReturnBB; - block->bbJumpDest->bbRefs++; + fgAddRefPred(genReturnBB, block); #ifdef DEBUG if (verbose) @@ -8531,7 +8566,7 @@ class MergedReturns newReturnBB->bbRefs = 1; // bbRefs gets update later, for now it should be 1 comp->fgReturnCount++; - newReturnBB->bbFlags |= BBF_INTERNAL; + newReturnBB->bbFlags |= (BBF_INTERNAL | BBF_JMP_TARGET); noway_assert(newReturnBB->bbNext == nullptr); @@ -13102,6 +13137,7 @@ void Compiler::fgComputeBlockAndEdgeWeights() const bool usingProfileWeights = fgIsUsingProfileWeights(); const bool isOptimizing = opts.OptimizationEnabled(); + fgModified = false; fgHaveValidEdgeWeights = false; fgCalledCount = BB_UNITY_WEIGHT; diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 02fd018ea45604..a1c6ee1728a93d 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -5483,6 +5483,18 @@ bool GenTree::OperMayThrow(Compiler* comp) case GT_ARR_ELEM: return comp->fgAddrCouldBeNull(this->AsArrElem()->gtArrObj); + case GT_FIELD: + { + GenTree* fldObj = this->AsField()->gtFldObj; + + if (fldObj != nullptr) + { + return comp->fgAddrCouldBeNull(fldObj); + } + + return false; + } + case GT_ARR_BOUNDS_CHECK: case GT_ARR_INDEX: case GT_ARR_OFFSET: diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 75ceb05772b5fe..9c821dc0905068 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -7232,6 +7232,11 @@ GenTree* Compiler::fgMorphPotentialTailCall(GenTreeCall* call) } #endif + // This block is not longer any block's predecessor. If we end up + // converting this tail call to a branch, we'll add appropriate + // successor information then. + fgRemoveBlockAsPred(compCurBB); + #if !FEATURE_TAILCALL_OPT_SHARED_RETURN // We enable shared-ret tail call optimization for recursive calls even if // FEATURE_TAILCALL_OPT_SHARED_RETURN is not defined. @@ -8033,6 +8038,7 @@ void Compiler::fgMorphRecursiveFastTailCallIntoLoop(BasicBlock* block, GenTreeCa fgFirstBB->bbFlags |= BBF_DONT_REMOVE; block->bbJumpKind = BBJ_ALWAYS; block->bbJumpDest = fgFirstBB->bbNext; + block->bbJumpDest->bbFlags |= BBF_JMP_TARGET; fgAddRefPred(block->bbJumpDest, block); block->bbFlags &= ~BBF_HAS_JMP; } @@ -14877,12 +14883,6 @@ bool Compiler::fgFoldConditional(BasicBlock* block) /* Unconditional throw - transform the basic block into a BBJ_THROW */ fgConvertBBToThrowBB(block); - /* Remove 'block' from the predecessor list of 'block->bbNext' */ - fgRemoveRefPred(block->bbNext, block); - - /* Remove 'block' from the predecessor list of 'block->bbJumpDest' */ - fgRemoveRefPred(block->bbJumpDest, block); - #ifdef DEBUG if (verbose) { @@ -15096,19 +15096,6 @@ bool Compiler::fgFoldConditional(BasicBlock* block) /* Unconditional throw - transform the basic block into a BBJ_THROW */ fgConvertBBToThrowBB(block); - /* update the flow graph */ - - unsigned jumpCnt = block->bbJumpSwt->bbsCount; - BasicBlock** jumpTab = block->bbJumpSwt->bbsDstTab; - - for (unsigned val = 0; val < jumpCnt; val++, jumpTab++) - { - BasicBlock* curJump = *jumpTab; - - /* Remove 'block' from the predecessor list of 'curJump' */ - fgRemoveRefPred(curJump, block); - } - #ifdef DEBUG if (verbose) { @@ -15207,22 +15194,30 @@ bool Compiler::fgFoldConditional(BasicBlock* block) return result; } -//***************************************************************************** +//------------------------------------------------------------------------ +// fgMorphBlockStmt: morph a single statement in a block. // -// Morphs a single statement in a block. -// Can be called anytime, unlike fgMorphStmts() which should only be called once. +// Arguments: +// block - block containing the statement +// stmt - statement to morph +// msg - string to identify caller in a dump // -// Returns true if 'stmt' was removed from the block. -// Returns false if 'stmt' is still in the block (even if other statements were removed). +// Returns: +// true if 'stmt' was removed from the block. +// s false if 'stmt' is still in the block (even if other statements were removed). +// +// Notes: +// Can be called anytime, unlike fgMorphStmts() which should only be called once. // - bool Compiler::fgMorphBlockStmt(BasicBlock* block, Statement* stmt DEBUGARG(const char* msg)) { assert(block != nullptr); assert(stmt != nullptr); - compCurBB = block; - compCurStmt = stmt; + // Reset some ambient state + fgRemoveRestOfBlock = false; + compCurBB = block; + compCurStmt = stmt; GenTree* morph = fgMorphTree(stmt->GetRootNode()); @@ -15315,10 +15310,7 @@ bool Compiler::fgMorphBlockStmt(BasicBlock* block, Statement* stmt DEBUGARG(cons } // The rest of block has been removed and we will always throw an exception. - - // Update succesors of block - fgRemoveBlockAsPred(block); - + // // For compDbgCode, we prepend an empty BB as the firstBB, it is BBJ_NONE. // We should not convert it to a ThrowBB. if ((block != fgFirstBB) || ((fgFirstBB->bbFlags & BBF_INTERNAL) == 0)) @@ -15682,6 +15674,7 @@ void Compiler::fgMorphBlocks() { block->bbJumpKind = BBJ_ALWAYS; block->bbJumpDest = genReturnBB; + fgAddRefPred(genReturnBB, block); fgReturnCount--; } if (genReturnLocal != BAD_VAR_NUM) @@ -16642,6 +16635,27 @@ void Compiler::fgMorph() EndPhase(PHASE_CLONE_FINALLY); + // Compute bbNum, bbRefs and bbPreds + // + JITDUMP("\nRenumbering the basic blocks for fgComputePreds\n"); + fgRenumberBlocks(); + + // This is the first time full (not cheap) preds will be computed + // + noway_assert(!fgComputePredsDone); + fgComputePreds(); + + // Run an early flow graph simplification pass + if (opts.OptimizationEnabled()) + { + fgUpdateFlowGraph(); + } + + EndPhase(PHASE_COMPUTE_PREDS); + + // From this point on the flowgraph information such as bbNum, + // bbRefs or bbPreds has to be kept updated + /* For x64 and ARM64 we need to mark irregular parameters */ lvaRefCountState = RCS_EARLY; fgResetImplicitByRefRefCount(); @@ -18842,18 +18856,34 @@ bool Compiler::fgCheckStmtAfterTailCall() #if FEATURE_TAILCALL_OPT_SHARED_RETURN - // We can have a move from the call result to an lvaInlineeReturnSpillTemp. - // However, we can't check that this assignment was created there. - if (nextMorphStmt->GetRootNode()->gtOper == GT_ASG) + // We can have a chain of assignments from the call result to + // various inline return spill temps. These are ok as long + // as the last one ultimately provides the return value or is ignored. + // + // And if we're returning a small type we may see a cast + // on the source side. + while ((nextMorphStmt != nullptr) && (nextMorphStmt->GetRootNode()->OperIs(GT_ASG))) { Statement* moveStmt = nextMorphStmt; GenTree* moveExpr = nextMorphStmt->GetRootNode(); - noway_assert(moveExpr->gtGetOp1()->OperIsLocal() && moveExpr->gtGetOp2()->OperIsLocal()); + GenTree* moveDest = moveExpr->gtGetOp1(); + noway_assert(moveDest->OperIsLocal()); + + // Tunnel through any casts on the source side. + GenTree* moveSource = moveExpr->gtGetOp2(); + while (moveSource->OperIs(GT_CAST)) + { + noway_assert(!moveSource->gtOverflow()); + moveSource = moveSource->gtGetOp1(); + } + noway_assert(moveSource->OperIsLocal()); - unsigned srcLclNum = moveExpr->gtGetOp2()->AsLclVarCommon()->GetLclNum(); + // Verify we're just passing the value from one local to another + // along the chain. + const unsigned srcLclNum = moveSource->AsLclVarCommon()->GetLclNum(); noway_assert(srcLclNum == callResultLclNumber); - unsigned dstLclNum = moveExpr->gtGetOp1()->AsLclVarCommon()->GetLclNum(); - callResultLclNumber = dstLclNum; + const unsigned dstLclNum = moveDest->AsLclVarCommon()->GetLclNum(); + callResultLclNumber = dstLclNum; nextMorphStmt = moveStmt->GetNextStmt(); } diff --git a/src/coreclr/src/jit/optimizer.cpp b/src/coreclr/src/jit/optimizer.cpp index 7b7b27667706ab..cddf3e1b63bb26 100644 --- a/src/coreclr/src/jit/optimizer.cpp +++ b/src/coreclr/src/jit/optimizer.cpp @@ -4073,6 +4073,13 @@ void Compiler::fgOptWhileLoop(BasicBlock* block) return; } + // block can't be the scratch bb, since we prefer to keep flow + // out of the scratch bb as BBJ_ALWAYS or BBJ_NONE. + if (fgBBisScratch(block)) + { + return; + } + // It has to be a forward jump // TODO-CQ: Check if we can also optimize the backwards jump as well. // diff --git a/src/coreclr/tests/src/JIT/opt/Tailcall/EarlyFlowOptExample.cs b/src/coreclr/tests/src/JIT/opt/Tailcall/EarlyFlowOptExample.cs new file mode 100644 index 00000000000000..4f56ea47987681 --- /dev/null +++ b/src/coreclr/tests/src/JIT/opt/Tailcall/EarlyFlowOptExample.cs @@ -0,0 +1,95 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Runtime.CompilerServices; + +// Test case showing that we can have "more complex" +// IR after a tail call if we do early flow opts. + +interface IX +{ +} + +class X : IX +{ + [MethodImpl(MethodImplOptions.NoInlining)] + bool P1(object o) => false; + + bool P0(object o) + { + bool b = false; + if (b) + { + return false; + } + return P1(o); + } + + // F3 needs to be too big to inline without being marked noinline, + // so that it ends up being tail called. + bool F3(object o) + { + int result = 0; + for (int i = 0; i < 100; i++) + { + result += i; + } + return result == 4950; + } + + // This method mainly serves to introduce are return spill temp + bool F2(object o) + { + bool b = false; + if (b) + { + return false; + } + return F3(o); + } + + // This method mainly serves to introduce are return spill temp + bool F1(object o) + { + if (P0(o)) + { + return false; + } + + return F2(o); + } + + // F0 is the method of interest. It will end up tail calling F3, + // and will initially have a chain of moves and casts after the + // call site which may trip up post tail call validation. + // + // We want F0 to be jitted, not inlined, but can't mark it as + // noinline or we'll also suppress tail calls. + bool F0(object o) + { + if (o == null) + { + return false; + } + + object ix = o as IX; + + if (ix == null) + { + return false; + } + + return F1(ix); + } + + // This stops F0 from being inlined + [MethodImpl(MethodImplOptions.NoOptimization)] + public static int Main() + { + X x = new X(); + bool b = x.F0(x); + return b ? 100 : -1; + } +} diff --git a/src/coreclr/tests/src/JIT/opt/Tailcall/EarlyFlowOptExample.csproj b/src/coreclr/tests/src/JIT/opt/Tailcall/EarlyFlowOptExample.csproj new file mode 100644 index 00000000000000..f3e1cbd44b4041 --- /dev/null +++ b/src/coreclr/tests/src/JIT/opt/Tailcall/EarlyFlowOptExample.csproj @@ -0,0 +1,12 @@ + + + Exe + + + None + True + + + + +