From 4b2ee6b8d38a3849dd50a84f02e99178f1025622 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 3 Jan 2020 14:24:33 -0800 Subject: [PATCH 01/11] JIT: build basic block pred lists before morph Build basic block pred lists before morph, instead of after, and add an early flow optimization pass. Fix up a few places where ref counts or pred lists were not properly maintained in morph. The early flow opt pass enhances the local assertion prop run in morph (by fusing blocks), allows the jit to avoid morphing some unreachable blocks (thus saving a bit of TP), and lays the groundwork for more aggressive early branch folding that would be useful eg dotnet/runtime#27935). --- src/coreclr/src/jit/compiler.cpp | 12 -------- src/coreclr/src/jit/compiler.hpp | 10 ++++++- src/coreclr/src/jit/flowgraph.cpp | 20 +++++++++---- src/coreclr/src/jit/morph.cpp | 49 ++++++++++++++++--------------- src/coreclr/src/jit/optimizer.cpp | 7 +++++ 5 files changed, 57 insertions(+), 41 deletions(-) diff --git a/src/coreclr/src/jit/compiler.cpp b/src/coreclr/src/jit/compiler.cpp index 6eacc9eff52fa0..3604b6d3cb1da9 100644 --- a/src/coreclr/src/jit/compiler.cpp +++ b/src/coreclr/src/jit/compiler.cpp @@ -4622,23 +4622,11 @@ 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"); - 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..fa0fab06ebb78b 100644 --- a/src/coreclr/src/jit/compiler.hpp +++ b/src/coreclr/src/jit/compiler.hpp @@ -2729,6 +2729,8 @@ inline unsigned Compiler::fgThrowHlpBlkStkLevel(BasicBlock* block) */ inline void Compiler::fgConvertBBToThrowBB(BasicBlock* block) { + JITDUMP("Converting " FMT_BB " to BBJ_THROW\n", block->bbNum); + // If we're converting a BBJ_CALLFINALLY block to a BBJ_THROW block, // then mark the subsequent BBJ_ALWAYS block as unreferenced. if (block->isBBCallAlwaysPair()) @@ -2758,8 +2760,14 @@ inline void Compiler::fgConvertBBToThrowBB(BasicBlock* block) #endif // defined(FEATURE_EH_FUNCLETS) && defined(_TARGET_ARM_) } + // Scrub this block from the pred lists of any successors + fgRemoveBlockAsPred(block); + + // Update jump kind after the scrub. block->bbJumpKind = BBJ_THROW; - block->bbSetRunRarely(); // any block with a throw is rare + + // Any block with a throw is rare + block->bbSetRunRarely(); } /***************************************************************************** diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp index 080e2d5cdf330b..4a0f84a78fb12e 100644 --- a/src/coreclr/src/jit/flowgraph.cpp +++ b/src/coreclr/src/jit/flowgraph.cpp @@ -431,9 +431,6 @@ BasicBlock* Compiler::fgNewBasicBlock(BBjumpKinds jumpKind) 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()) @@ -453,6 +450,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 +472,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 @@ -8244,7 +8253,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 +8540,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 +13111,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/morph.cpp b/src/coreclr/src/jit/morph.cpp index 75ceb05772b5fe..ec86708fecb613 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. @@ -14877,12 +14882,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 +15095,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) { @@ -15315,10 +15301,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 +15665,7 @@ void Compiler::fgMorphBlocks() { block->bbJumpKind = BBJ_ALWAYS; block->bbJumpDest = genReturnBB; + fgAddRefPred(genReturnBB, block); fgReturnCount--; } if (genReturnLocal != BAD_VAR_NUM) @@ -16642,6 +16626,25 @@ void Compiler::fgMorph() EndPhase(PHASE_CLONE_FINALLY); + /* Compute bbNum, bbRefs and bbPreds */ + + JITDUMP("\nRenumbering the basic blocks for fgComputePreds\n"); + fgRenumberBlocks(); + + noway_assert(!fgComputePredsDone); // This is the first time full (not cheap) preds will be computed. + 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(); 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. // From 300c1df64f456d13b0cb721641a7a61c1676d960 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 6 Jan 2020 12:03:18 -0800 Subject: [PATCH 02/11] fix recursive tail call to loop case --- src/coreclr/src/jit/morph.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index ec86708fecb613..f79aa5d6718503 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -8038,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; } From 10257d854fe1661be604769ed3c0774e8de85555 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 6 Jan 2020 15:48:43 -0800 Subject: [PATCH 03/11] don't build address modes out of handles --- src/coreclr/src/jit/codegencommon.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/coreclr/src/jit/codegencommon.cpp b/src/coreclr/src/jit/codegencommon.cpp index 7eb808f2c64c34..2c3fd7bbc8f6d3 100644 --- a/src/coreclr/src/jit/codegencommon.cpp +++ b/src/coreclr/src/jit/codegencommon.cpp @@ -1375,6 +1375,12 @@ bool CodeGen::genCreateAddrMode(GenTree* addr, if (op2->IsIntCnsFitsInI32() && (op2->gtType != TYP_REF) && FitsIn(cns + op2->AsIntConCommon()->IconValue())) { + // Don't build address modes out of non-foldable constants + if (!op2->AsIntConCommon()->ImmedValCanBeFolded(compiler, addr->OperGet())) + { + return false; + } + /* We're adding a constant */ cns += op2->AsIntConCommon()->IconValue(); From b21a56d3ce59fceedb37a7e4a94313c70d1e3550 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 7 Jan 2020 10:48:09 -0800 Subject: [PATCH 04/11] GT_FIELD may throw --- src/coreclr/src/jit/gentree.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) 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: From 18f1ae123662700f90cabbd8c7a6fad649244b9a Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 8 Jan 2020 02:04:10 -0800 Subject: [PATCH 05/11] clean finally target bit properly --- src/coreclr/src/jit/compiler.hpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/coreclr/src/jit/compiler.hpp b/src/coreclr/src/jit/compiler.hpp index fa0fab06ebb78b..7f207e564eaf98 100644 --- a/src/coreclr/src/jit/compiler.hpp +++ b/src/coreclr/src/jit/compiler.hpp @@ -2738,10 +2738,6 @@ inline void Compiler::fgConvertBBToThrowBB(BasicBlock* block) BasicBlock* leaveBlk = block->bbNext; noway_assert(leaveBlk->bbJumpKind == BBJ_ALWAYS); - leaveBlk->bbFlags &= ~BBF_DONT_REMOVE; - leaveBlk->bbRefs = 0; - leaveBlk->bbPreds = nullptr; - #if defined(FEATURE_EH_FUNCLETS) && defined(_TARGET_ARM_) // This function (fgConvertBBToThrowBB) can be called before the predecessor lists are created (e.g., in // fgMorph). The fgClearFinallyTargetBit() function to update the BBF_FINALLY_TARGET bit depends on these @@ -2758,6 +2754,12 @@ inline void Compiler::fgConvertBBToThrowBB(BasicBlock* block) fgNeedToAddFinallyTargetBits = true; } #endif // defined(FEATURE_EH_FUNCLETS) && defined(_TARGET_ARM_) + + // leaveBlk is now unreachable, so scrub the pred lists. + // Note this must happen after resetting finally target bits. + leaveBlk->bbFlags &= ~BBF_DONT_REMOVE; + leaveBlk->bbRefs = 0; + leaveBlk->bbPreds = nullptr; } // Scrub this block from the pred lists of any successors From e1ca6b715be1a7ccaae91ba25c8533eb6a7bd2c3 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 8 Jan 2020 09:35:58 -0800 Subject: [PATCH 06/11] clear callfinally target bit properly, take two --- src/coreclr/src/jit/compiler.hpp | 39 ++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/src/coreclr/src/jit/compiler.hpp b/src/coreclr/src/jit/compiler.hpp index 7f207e564eaf98..457a16b4eae443 100644 --- a/src/coreclr/src/jit/compiler.hpp +++ b/src/coreclr/src/jit/compiler.hpp @@ -2731,13 +2731,33 @@ inline void Compiler::fgConvertBBToThrowBB(BasicBlock* block) { JITDUMP("Converting " FMT_BB " to BBJ_THROW\n", block->bbNum); - // If we're converting a BBJ_CALLFINALLY block to a BBJ_THROW block, + // 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; + #if defined(FEATURE_EH_FUNCLETS) && defined(_TARGET_ARM_) // This function (fgConvertBBToThrowBB) can be called before the predecessor lists are created (e.g., in // fgMorph). The fgClearFinallyTargetBit() function to update the BBF_FINALLY_TARGET bit depends on these @@ -2754,22 +2774,7 @@ inline void Compiler::fgConvertBBToThrowBB(BasicBlock* block) fgNeedToAddFinallyTargetBits = true; } #endif // defined(FEATURE_EH_FUNCLETS) && defined(_TARGET_ARM_) - - // leaveBlk is now unreachable, so scrub the pred lists. - // Note this must happen after resetting finally target bits. - leaveBlk->bbFlags &= ~BBF_DONT_REMOVE; - leaveBlk->bbRefs = 0; - leaveBlk->bbPreds = nullptr; } - - // 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(); } /***************************************************************************** From 30d6c9d34b041d1dec429ec400b830498ad91cf7 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 8 Jan 2020 14:17:26 -0800 Subject: [PATCH 07/11] fix fgMorphBlockStmt --- src/coreclr/src/jit/morph.cpp | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index f79aa5d6718503..244b7f1c16c29a 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -15194,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()); From eee522a8d198963a6972b99eb71ac9ba8ab17c49 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 9 Jan 2020 10:26:09 -0800 Subject: [PATCH 08/11] review feedback --- src/coreclr/src/jit/flowgraph.cpp | 48 ++++++++++++++++++++++++------- src/coreclr/src/jit/morph.cpp | 12 ++++---- 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp index 4a0f84a78fb12e..d4be3d96414dd0 100644 --- a/src/coreclr/src/jit/flowgraph.cpp +++ b/src/coreclr/src/jit/flowgraph.cpp @@ -419,20 +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() { // Have we already allocated a scratch block? - if (fgFirstBBisScratch()) { return; @@ -485,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) @@ -506,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); diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 244b7f1c16c29a..460b46c83a1cb7 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -16635,12 +16635,14 @@ void Compiler::fgMorph() EndPhase(PHASE_CLONE_FINALLY); - /* Compute bbNum, bbRefs and bbPreds */ - + // Compute bbNum, bbRefs and bbPreds + // JITDUMP("\nRenumbering the basic blocks for fgComputePreds\n"); fgRenumberBlocks(); - noway_assert(!fgComputePredsDone); // This is the first time full (not cheap) preds will be computed. + // This is the first time full (not cheap) preds will be computed + // + noway_assert(!fgComputePredsDone); fgComputePreds(); // Run an early flow graph simplification pass @@ -16651,8 +16653,8 @@ void Compiler::fgMorph() EndPhase(PHASE_COMPUTE_PREDS); - /* From this point on the flowgraph information such as bbNum, - * bbRefs or bbPreds has to be kept updated */ + // 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; From 620035afb880bec71d804a2715870299fa20754b Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 10 Jan 2020 10:34:20 -0800 Subject: [PATCH 09/11] undo change to lea formation in favor of #1593 --- src/coreclr/src/jit/codegencommon.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/coreclr/src/jit/codegencommon.cpp b/src/coreclr/src/jit/codegencommon.cpp index 2c3fd7bbc8f6d3..7eb808f2c64c34 100644 --- a/src/coreclr/src/jit/codegencommon.cpp +++ b/src/coreclr/src/jit/codegencommon.cpp @@ -1375,12 +1375,6 @@ bool CodeGen::genCreateAddrMode(GenTree* addr, if (op2->IsIntCnsFitsInI32() && (op2->gtType != TYP_REF) && FitsIn(cns + op2->AsIntConCommon()->IconValue())) { - // Don't build address modes out of non-foldable constants - if (!op2->AsIntConCommon()->ImmedValCanBeFolded(compiler, addr->OperGet())) - { - return false; - } - /* We're adding a constant */ cns += op2->AsIntConCommon()->IconValue(); From c96681545c4e2f26b56b536187eddd94ea3235c9 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 10 Jan 2020 14:49:00 -0800 Subject: [PATCH 10/11] more general post tail call validation, plus test case --- src/coreclr/src/jit/morph.cpp | 30 ++++-- .../JIT/opt/Tailcall/EarlyFlowOptExample.cs | 95 +++++++++++++++++++ .../opt/Tailcall/EarlyFlowOptExample.csproj | 12 +++ 3 files changed, 130 insertions(+), 7 deletions(-) create mode 100644 src/coreclr/tests/src/JIT/opt/Tailcall/EarlyFlowOptExample.cs create mode 100644 src/coreclr/tests/src/JIT/opt/Tailcall/EarlyFlowOptExample.csproj diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 460b46c83a1cb7..9c821dc0905068 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -18856,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/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 + + + + + From d04dbff98fd248df3b2dc7a931139c4719ec649c Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 10 Jan 2020 23:57:17 -0800 Subject: [PATCH 11/11] fgMarkGCPollBlocks needs renumbered BBs --- src/coreclr/src/jit/compiler.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/coreclr/src/jit/compiler.cpp b/src/coreclr/src/jit/compiler.cpp index 3604b6d3cb1da9..1731dc4c7ada61 100644 --- a/src/coreclr/src/jit/compiler.cpp +++ b/src/coreclr/src/jit/compiler.cpp @@ -4622,8 +4622,13 @@ void Compiler::compCompile(void** methodCodePtr, ULONG* methodCodeSize, JitFlags } EndPhase(PHASE_GS_COOKIE); + // GC Poll marking assumes block bbnums match lexical block order, + // so make sure this is the case. + fgRenumberBlocks(); + /* 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);