From c2ae4dd59a4018152490899af6699a7ae879eb1c Mon Sep 17 00:00:00 2001 From: Carol Eidt Date: Wed, 13 Nov 2019 15:27:44 -0800 Subject: [PATCH] - Change `gtIsLikelyRegVar` to return false for defs of EHvars, since they always go to memory. - Never split a block if the only resolution moves are for EH vars. - Add a test case to enable diff tracking for #21973 --- src/jit/gentree.cpp | 7 +++ src/jit/lsra.cpp | 25 +++++++- src/jit/lsrabuild.cpp | 63 +++++++++---------- .../JitBlue/GitHub_21973/GitHub_21973.cs | 24 +++++++ .../JitBlue/GitHub_21973/GitHub_21973.csproj | 13 ++++ 5 files changed, 98 insertions(+), 34 deletions(-) create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_21973/GitHub_21973.cs create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_21973/GitHub_21973.csproj diff --git a/src/jit/gentree.cpp b/src/jit/gentree.cpp index bb46c8f7b6a5..2a421a8c0d0c 100644 --- a/src/jit/gentree.cpp +++ b/src/jit/gentree.cpp @@ -2755,6 +2755,13 @@ bool Compiler::gtIsLikelyRegVar(GenTree* tree) return false; } + // If this is an EH-live var, return false if it is a def, + // as it will have to go to memory. + if (varDsc->lvLiveInOutOfHndlr && ((tree->gtFlags & GTF_VAR_DEF) != 0)) + { + return false; + } + // Be pessimistic if ref counts are not yet set up. // // Perhaps we should be optimistic though. diff --git a/src/jit/lsra.cpp b/src/jit/lsra.cpp index 2f1693f8efa9..fd7db2aefea0 100644 --- a/src/jit/lsra.cpp +++ b/src/jit/lsra.cpp @@ -8232,7 +8232,30 @@ void LinearScan::handleOutgoingCriticalEdges(BasicBlock* block) } if (!VarSetOps::IsEmpty(compiler, edgeResolutionSet)) { - resolveEdge(block, succBlock, ResolveCritical, edgeResolutionSet); + // For EH vars, we can always safely load them from the stack into the target for this block, + // so if we have only EH vars, we'll do that instead of splitting the edge. + if ((compiler->compHndBBtabCount > 0) && VarSetOps::IsSubset(compiler, edgeResolutionSet, exceptVars)) + { + GenTree* insertionPoint = LIR::AsRange(succBlock).FirstNonPhiNode(); + VarSetOps::Iter edgeSetIter(compiler, edgeResolutionSet); + unsigned edgeVarIndex = 0; + while (edgeSetIter.NextElem(&edgeVarIndex)) + { + regNumber toReg = getVarReg(succInVarToRegMap, edgeVarIndex); + setVarReg(succInVarToRegMap, edgeVarIndex, REG_STK); + if (toReg != REG_STK) + { + Interval* interval = getIntervalForLocalVar(edgeVarIndex); + assert(interval->isWriteThru); + addResolution(succBlock, insertionPoint, interval, toReg, REG_STK); + JITDUMP(" (EHvar)\n"); + } + } + } + else + { + resolveEdge(block, succBlock, ResolveCritical, edgeResolutionSet); + } } } } diff --git a/src/jit/lsrabuild.cpp b/src/jit/lsrabuild.cpp index fe80cbc9d7a9..d6cfce072761 100644 --- a/src/jit/lsrabuild.cpp +++ b/src/jit/lsrabuild.cpp @@ -2248,42 +2248,38 @@ void LinearScan::buildIntervals() if (enregisterLocalVars) { - // We don't need exposed uses for an EH edge, because no lclVars will be kept in - // registers across such edges. - if (!blockInfo[block->bbNum].hasEHBoundaryOut) + // Insert exposed uses for a lclVar that is live-out of 'block' but not live-in to the + // next block, or any unvisited successors. + // This will address lclVars that are live on a backedge, as well as those that are kept + // live at a GT_JMP. + // + // Blocks ending with "jmp method" are marked as BBJ_HAS_JMP, + // and jmp call is represented using GT_JMP node which is a leaf node. + // Liveness phase keeps all the arguments of the method live till the end of + // block by adding them to liveout set of the block containing GT_JMP. + // + // The target of a GT_JMP implicitly uses all the current method arguments, however + // there are no actual references to them. This can cause LSRA to assert, because + // the variables are live but it sees no references. In order to correctly model the + // liveness of these arguments, we add dummy exposed uses, in the same manner as for + // backward branches. This will happen automatically via expUseSet. + // + // Note that a block ending with GT_JMP has no successors and hence the variables + // for which dummy use ref positions are added are arguments of the method. + + VARSET_TP expUseSet(VarSetOps::MakeCopy(compiler, block->bbLiveOut)); + VarSetOps::IntersectionD(compiler, expUseSet, registerCandidateVars); + BasicBlock* nextBlock = getNextBlock(); + if (nextBlock != nullptr) { - // Insert exposed uses for a lclVar that is live-out of 'block' but not live-in to the - // next block, or any unvisited successors. - // This will address lclVars that are live on a backedge, as well as those that are kept - // live at a GT_JMP. - // - // Blocks ending with "jmp method" are marked as BBJ_HAS_JMP, - // and jmp call is represented using GT_JMP node which is a leaf node. - // Liveness phase keeps all the arguments of the method live till the end of - // block by adding them to liveout set of the block containing GT_JMP. - // - // The target of a GT_JMP implicitly uses all the current method arguments, however - // there are no actual references to them. This can cause LSRA to assert, because - // the variables are live but it sees no references. In order to correctly model the - // liveness of these arguments, we add dummy exposed uses, in the same manner as for - // backward branches. This will happen automatically via expUseSet. - // - // Note that a block ending with GT_JMP has no successors and hence the variables - // for which dummy use ref positions are added are arguments of the method. - - VARSET_TP expUseSet(VarSetOps::MakeCopy(compiler, block->bbLiveOut)); - VarSetOps::IntersectionD(compiler, expUseSet, registerCandidateVars); - BasicBlock* nextBlock = getNextBlock(); - if (nextBlock != nullptr) + VarSetOps::DiffD(compiler, expUseSet, nextBlock->bbLiveIn); + } + for (BasicBlock* succ : block->GetAllSuccs(compiler)) + { + if (VarSetOps::IsEmpty(compiler, expUseSet)) { - VarSetOps::DiffD(compiler, expUseSet, nextBlock->bbLiveIn); + break; } - for (BasicBlock* succ : block->GetAllSuccs(compiler)) - { - if (VarSetOps::IsEmpty(compiler, expUseSet)) - { - break; - } if (isBlockVisited(succ)) { @@ -2291,6 +2287,7 @@ void LinearScan::buildIntervals() } VarSetOps::DiffD(compiler, expUseSet, succ->bbLiveIn); } + if (!VarSetOps::IsEmpty(compiler, expUseSet)) { JITDUMP("Exposed uses:"); diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_21973/GitHub_21973.cs b/tests/src/JIT/Regression/JitBlue/GitHub_21973/GitHub_21973.cs new file mode 100644 index 000000000000..5656c2ccaff0 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_21973/GitHub_21973.cs @@ -0,0 +1,24 @@ +// 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.Threading.Tasks; +using System.Runtime.CompilerServices; + +public class Test +{ + + [MethodImpl(MethodImplOptions.NoInlining)] + public static async Task CompletedTask() + { + for (int i = 0; i < 100; i++) + await Task.CompletedTask; + } + + public static int Main() + { + CompletedTask(); + return 100; + } +} diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_21973/GitHub_21973.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_21973/GitHub_21973.csproj new file mode 100644 index 000000000000..656bedde1024 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_21973/GitHub_21973.csproj @@ -0,0 +1,13 @@ + + + Exe + 1 + + + + True + + + + +