From: Joseph Tremoulet Date: Wed, 11 Jan 2017 19:58:57 +0000 (-0500) Subject: Define `MemoryKind::ByrefExposed` X-Git-Tag: submit/tizen/20210909.063632~11030^2~8168^2~1 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=d7c952dba010257db1b29f3b111f131e4d8ccb35;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Define `MemoryKind::ByrefExposed` Add a new `MemoryKind` to represent "any memory that byrefs may reference", called `ByrefExposed`. All definition points of `GcHeap` become also definition points of `ByrefExposed`, and additionally, so do definitions of address-exposed locals. Because it is a common case (currently happening in 90% of methods in System.Private.CoreLib) that a method has no definitions of address-exposed locals, have liveness detect this condition and set the new `byrefStatesMatchGcHeapStates` flag accordingly; when the states match, the same `MemoryPhi`s, defnums, and `PerSsaData` are then shared between the two memory kinds, to avoid excess allocations/processing. This change defines `ByrefExposed` memory and its def/use points, and builds SSA for it, but no optimizations make use of it, so this is a no-diff change. Commit migrated from https://github.com/dotnet/coreclr/commit/1c6a419710ed292f2fa48e459239ee95e3ed2ca2 --- diff --git a/src/coreclr/src/jit/block.h b/src/coreclr/src/jit/block.h index d95eb9c..786b831 100644 --- a/src/coreclr/src/jit/block.h +++ b/src/coreclr/src/jit/block.h @@ -147,11 +147,13 @@ struct EntryState // Enumeration of the kinds of memory whose state changes the compiler tracks enum MemoryKind { - GcHeap = 0, // Includes actual GC heap, and also static fields - MemoryKindCount, // Number of MemoryKinds + ByrefExposed = 0, // Includes anything byrefs can read/write (everything in GcHeap, address-taken locals, + // unmanaged heap, callers' locals, etc.) + GcHeap, // Includes actual GC heap, and also static fields + MemoryKindCount, // Number of MemoryKinds }; #ifdef DEBUG -const char* const memoryKindNames[] = {"GcHeap"}; +const char* const memoryKindNames[] = {"ByrefExposed", "GcHeap"}; #endif // DEBUG // Bitmask describing a set of memory kinds (usable in bitfields) diff --git a/src/coreclr/src/jit/codegenlegacy.cpp b/src/coreclr/src/jit/codegenlegacy.cpp index a57953a..5605372 100644 --- a/src/coreclr/src/jit/codegenlegacy.cpp +++ b/src/coreclr/src/jit/codegenlegacy.cpp @@ -20743,39 +20743,39 @@ GenTreePtr Compiler::fgLegacyPerStatementLocalVarLiveness(GenTreePtr startNode, break; case GT_CLS_VAR: - // For Volatile indirection, first mutate GcHeap + // For Volatile indirection, first mutate GcHeap/ByrefExposed // see comments in ValueNum.cpp (under case GT_CLS_VAR) // This models Volatile reads as def-then-use of the heap. // and allows for a CSE of a subsequent non-volatile read if ((tree->gtFlags & GTF_FLD_VOLATILE) != 0) { // For any Volatile indirection, we must handle it as a - // definition of GcHeap - fgCurMemoryDef |= memoryKindSet(GcHeap); + // definition of GcHeap/ByrefExposed + fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed); } // If the GT_CLS_VAR is the lhs of an assignment, we'll handle it as a heap def, when we get to // assignment. // Otherwise, we treat it as a use here. if ((tree->gtFlags & GTF_CLS_VAR_ASG_LHS) == 0) { - fgCurMemoryUse |= memoryKindSet(GcHeap); + fgCurMemoryUse |= memoryKindSet(GcHeap, ByrefExposed); } break; case GT_IND: - // For Volatile indirection, first mutate GcHeap + // For Volatile indirection, first mutate GcHeap/ByrefExposed // see comments in ValueNum.cpp (under case GT_CLS_VAR) // This models Volatile reads as def-then-use of the heap. // and allows for a CSE of a subsequent non-volatile read if ((tree->gtFlags & GTF_IND_VOLATILE) != 0) { // For any Volatile indirection, we must handle it as a - // definition of GcHeap - fgCurMemoryDef |= memoryKindSet(GcHeap); + // definition of GcHeap/ByrefExposed + fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed); } // If the GT_IND is the lhs of an assignment, we'll handle it - // as a heap def, when we get to assignment. + // as a heap/byref def, when we get to assignment. // Otherwise, we treat it as a use here. if ((tree->gtFlags & GTF_IND_ASG_LHS) == 0) { @@ -20784,7 +20784,7 @@ GenTreePtr Compiler::fgLegacyPerStatementLocalVarLiveness(GenTreePtr startNode, GenTreePtr addrArg = tree->gtOp.gtOp1->gtEffectiveVal(/*commaOnly*/ true); if (!addrArg->DefinesLocalAddr(this, /*width doesn't matter*/ 0, &dummyLclVarTree, &dummyIsEntire)) { - fgCurMemoryUse |= memoryKindSet(GcHeap); + fgCurMemoryUse |= memoryKindSet(GcHeap, ByrefExposed); } else { @@ -20801,22 +20801,23 @@ GenTreePtr Compiler::fgLegacyPerStatementLocalVarLiveness(GenTreePtr startNode, unreached(); break; - // We'll assume these are use-then-defs of GcHeap. + // We'll assume these are use-then-defs of GcHeap/ByrefExposed. case GT_LOCKADD: case GT_XADD: case GT_XCHG: case GT_CMPXCHG: - fgCurMemoryUse |= memoryKindSet(GcHeap); - fgCurMemoryDef |= memoryKindSet(GcHeap); - fgCurMemoryHavoc |= memoryKindSet(GcHeap); + fgCurMemoryUse |= memoryKindSet(GcHeap, ByrefExposed); + fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed); + fgCurMemoryHavoc |= memoryKindSet(GcHeap, ByrefExposed); break; case GT_MEMORYBARRIER: - // Simliar to any Volatile indirection, we must handle this as a definition of GcHeap - fgCurMemoryDef |= memoryKindSet(GcHeap); + // Simliar to any Volatile indirection, we must handle this as a definition of GcHeap/ByrefExposed + fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed); break; - // For now, all calls read/write GcHeap, the latter in its entirety. Might tighten this case later. + // For now, all calls read/write GcHeap/ByrefExposed, writes in their entirety. Might tighten this case + // later. case GT_CALL: { GenTreeCall* call = tree->AsCall(); @@ -20832,9 +20833,9 @@ GenTreePtr Compiler::fgLegacyPerStatementLocalVarLiveness(GenTreePtr startNode, } if (modHeap) { - fgCurMemoryUse |= memoryKindSet(GcHeap); - fgCurMemoryDef |= memoryKindSet(GcHeap); - fgCurMemoryHavoc |= memoryKindSet(GcHeap); + fgCurMemoryUse |= memoryKindSet(GcHeap, ByrefExposed); + fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed); + fgCurMemoryHavoc |= memoryKindSet(GcHeap, ByrefExposed); } } @@ -20866,14 +20867,26 @@ GenTreePtr Compiler::fgLegacyPerStatementLocalVarLiveness(GenTreePtr startNode, default: - // Determine whether it defines a GC heap location. + // Determine what memory kinds it defines. if (tree->OperIsAssignment() || tree->OperIsBlkOp()) { GenTreeLclVarCommon* dummyLclVarTree = NULL; - if (!tree->DefinesLocal(this, &dummyLclVarTree)) + if (tree->DefinesLocal(this, &dummyLclVarTree)) { - // If it doesn't define a local, then it might update the GC heap. - fgCurMemoryDef |= memoryKindSet(GcHeap); + if (lvaVarAddrExposed(dummyLclVarTree->gtLclNum)) + { + fgCurMemoryDef |= memoryKindSet(ByrefExposed); + + // We've found a store that modifies ByrefExposed + // memory but not GcHeap memory, so track their + // states separately. + byrefStatesMatchGcHeapStates = false; + } + } + else + { + // If it doesn't define a local, then it might update GcHeap/ByrefExposed. + fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed); } } diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 4862499..1fb8325 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -4678,6 +4678,8 @@ private: MemoryKindSet fgCurMemoryDef; // True iff the current basic block modifies memory. MemoryKindSet fgCurMemoryHavoc; // True if the current basic block is known to set memory to a "havoc" value. + bool byrefStatesMatchGcHeapStates; // True iff GcHeap and ByrefExposed memory have all the same def points. + void fgMarkUseDef(GenTreeLclVarCommon* tree); void fgBeginScopeLife(VARSET_TP* inScope, VarScopeDsc* var); @@ -9036,6 +9038,12 @@ public: // state, all the possible memory states are possible initial states of the corresponding catch block(s).) NodeToUnsignedMap* GetMemorySsaMap(MemoryKind memoryKind) { + if (memoryKind == GcHeap && byrefStatesMatchGcHeapStates) + { + // Use the same map for GCHeap and ByrefExposed when their states match. + memoryKind = ByrefExposed; + } + assert(memoryKind < MemoryKindCount); Compiler* compRoot = impInlineRoot(); if (compRoot->m_memorySsaMap[memoryKind] == nullptr) diff --git a/src/coreclr/src/jit/liveness.cpp b/src/coreclr/src/jit/liveness.cpp index bf81bcd..c666318 100644 --- a/src/coreclr/src/jit/liveness.cpp +++ b/src/coreclr/src/jit/liveness.cpp @@ -37,12 +37,17 @@ void Compiler::fgMarkUseDef(GenTreeLclVarCommon* tree) varDsc->lvRefCnt = 1; } + const bool isDef = (tree->gtFlags & GTF_VAR_DEF) != 0; + const bool isUse = !isDef || ((tree->gtFlags & (GTF_VAR_USEASG | GTF_VAR_USEDEF)) != 0); + if (varDsc->lvTracked) { assert(varDsc->lvVarIndex < lvaTrackedCount); - const bool isDef = (tree->gtFlags & GTF_VAR_DEF) != 0; - const bool isUse = !isDef || ((tree->gtFlags & (GTF_VAR_USEASG | GTF_VAR_USEDEF)) != 0); + // We don't treat stores to tracked locals as modifications of ByrefExposed memory; + // Make sure no tracked local is addr-exposed, to make sure we don't incorrectly CSE byref + // loads aliasing it across a store to it. + assert(!varDsc->lvAddrExposed); if (isUse && !VarSetOps::IsMember(this, fgCurDefSet, varDsc->lvVarIndex)) { @@ -56,33 +61,56 @@ void Compiler::fgMarkUseDef(GenTreeLclVarCommon* tree) VarSetOps::AddElemD(this, fgCurDefSet, varDsc->lvVarIndex); } } - else if (varTypeIsStruct(varDsc)) + else { - lvaPromotionType promotionType = lvaGetPromotionType(varDsc); - - if (promotionType != PROMOTION_TYPE_NONE) + if (varDsc->lvAddrExposed) { - VARSET_TP VARSET_INIT_NOCOPY(bitMask, VarSetOps::MakeEmpty(this)); + // Reflect the effect on ByrefExposed memory - for (unsigned i = varDsc->lvFieldLclStart; i < varDsc->lvFieldLclStart + varDsc->lvFieldCnt; ++i) + if (isUse) { - noway_assert(lvaTable[i].lvIsStructField); - if (lvaTable[i].lvTracked) - { - noway_assert(lvaTable[i].lvVarIndex < lvaTrackedCount); - VarSetOps::AddElemD(this, bitMask, lvaTable[i].lvVarIndex); - } + fgCurMemoryUse |= memoryKindSet(ByrefExposed); } - - // For pure defs (i.e. not an "update" def which is also a use), add to the (all) def set. - if ((tree->gtFlags & GTF_VAR_DEF) != 0 && (tree->gtFlags & (GTF_VAR_USEASG | GTF_VAR_USEDEF)) == 0) + if (isDef) { - VarSetOps::UnionD(this, fgCurDefSet, bitMask); + fgCurMemoryDef |= memoryKindSet(ByrefExposed); + + // We've found a store that modifies ByrefExposed + // memory but not GcHeap memory, so track their + // states separately. + byrefStatesMatchGcHeapStates = false; } - else if (!VarSetOps::IsSubset(this, bitMask, fgCurDefSet)) + } + + if (varTypeIsStruct(varDsc)) + { + lvaPromotionType promotionType = lvaGetPromotionType(varDsc); + + if (promotionType != PROMOTION_TYPE_NONE) { - // Mark as used any struct fields that are not yet defined. - VarSetOps::UnionD(this, fgCurUseSet, bitMask); + VARSET_TP VARSET_INIT_NOCOPY(bitMask, VarSetOps::MakeEmpty(this)); + + for (unsigned i = varDsc->lvFieldLclStart; i < varDsc->lvFieldLclStart + varDsc->lvFieldCnt; ++i) + { + noway_assert(lvaTable[i].lvIsStructField); + if (lvaTable[i].lvTracked) + { + noway_assert(lvaTable[i].lvVarIndex < lvaTrackedCount); + VarSetOps::AddElemD(this, bitMask, lvaTable[i].lvVarIndex); + } + } + + // For pure defs (i.e. not an "update" def which is also a use), add to the (all) def set. + if (!isUse) + { + assert(isDef); + VarSetOps::UnionD(this, fgCurDefSet, bitMask); + } + else if (!VarSetOps::IsSubset(this, bitMask, fgCurDefSet)) + { + // Mark as used any struct fields that are not yet defined. + VarSetOps::UnionD(this, fgCurUseSet, bitMask); + } } } } @@ -225,35 +253,35 @@ void Compiler::fgPerNodeLocalVarLiveness(GenTree* tree) break; case GT_CLS_VAR: - // For Volatile indirection, first mutate GcHeap. + // For Volatile indirection, first mutate GcHeap/ByrefExposed. // See comments in ValueNum.cpp (under case GT_CLS_VAR) // This models Volatile reads as def-then-use of memory // and allows for a CSE of a subsequent non-volatile read. if ((tree->gtFlags & GTF_FLD_VOLATILE) != 0) { // For any Volatile indirection, we must handle it as a - // definition of GcHeap - fgCurMemoryDef |= memoryKindSet(GcHeap); + // definition of GcHeap/ByrefExposed + fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed); } - // If the GT_CLS_VAR is the lhs of an assignment, we'll handle it as a GcHeap def, when we get to - // assignment. + // If the GT_CLS_VAR is the lhs of an assignment, we'll handle it as a GcHeap/ByrefExposed def, when we get + // to the assignment. // Otherwise, we treat it as a use here. if ((tree->gtFlags & GTF_CLS_VAR_ASG_LHS) == 0) { - fgCurMemoryUse |= memoryKindSet(GcHeap); + fgCurMemoryUse |= memoryKindSet(GcHeap, ByrefExposed); } break; case GT_IND: - // For Volatile indirection, first mutate GcHeap + // For Volatile indirection, first mutate GcHeap/ByrefExposed // see comments in ValueNum.cpp (under case GT_CLS_VAR) // This models Volatile reads as def-then-use of memory. // and allows for a CSE of a subsequent non-volatile read if ((tree->gtFlags & GTF_IND_VOLATILE) != 0) { // For any Volatile indirection, we must handle it as a - // definition of the GcHeap - fgCurMemoryDef |= memoryKindSet(GcHeap); + // definition of the GcHeap/ByrefExposed + fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed); } // If the GT_IND is the lhs of an assignment, we'll handle it @@ -266,7 +294,7 @@ void Compiler::fgPerNodeLocalVarLiveness(GenTree* tree) GenTreePtr addrArg = tree->gtOp.gtOp1->gtEffectiveVal(/*commaOnly*/ true); if (!addrArg->DefinesLocalAddr(this, /*width doesn't matter*/ 0, &dummyLclVarTree, &dummyIsEntire)) { - fgCurMemoryUse |= memoryKindSet(GcHeap); + fgCurMemoryUse |= memoryKindSet(GcHeap, ByrefExposed); } else { @@ -288,17 +316,17 @@ void Compiler::fgPerNodeLocalVarLiveness(GenTree* tree) case GT_XADD: case GT_XCHG: case GT_CMPXCHG: - fgCurMemoryUse |= memoryKindSet(GcHeap); - fgCurMemoryDef |= memoryKindSet(GcHeap); - fgCurMemoryHavoc |= memoryKindSet(GcHeap); + fgCurMemoryUse |= memoryKindSet(GcHeap, ByrefExposed); + fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed); + fgCurMemoryHavoc |= memoryKindSet(GcHeap, ByrefExposed); break; case GT_MEMORYBARRIER: - // Simliar to any Volatile indirection, we must handle this as a definition of GcHeap - fgCurMemoryDef |= memoryKindSet(GcHeap); + // Simliar to any Volatile indirection, we must handle this as a definition of GcHeap/ByrefExposed + fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed); break; - // For now, all calls read/write GcHeap, the latter in its entirety. Might tighten this case later. + // For now, all calls read/write GcHeap/ByrefExposed, writes in their entirety. Might tighten this case later. case GT_CALL: { GenTreeCall* call = tree->AsCall(); @@ -314,9 +342,9 @@ void Compiler::fgPerNodeLocalVarLiveness(GenTree* tree) } if (modHeap) { - fgCurMemoryUse |= memoryKindSet(GcHeap); - fgCurMemoryDef |= memoryKindSet(GcHeap); - fgCurMemoryHavoc |= memoryKindSet(GcHeap); + fgCurMemoryUse |= memoryKindSet(GcHeap, ByrefExposed); + fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed); + fgCurMemoryHavoc |= memoryKindSet(GcHeap, ByrefExposed); } } @@ -352,14 +380,26 @@ void Compiler::fgPerNodeLocalVarLiveness(GenTree* tree) default: - // Determine whether it defines a GcHeap location. + // Determine what memory locations it defines. if (tree->OperIsAssignment() || tree->OperIsBlkOp()) { GenTreeLclVarCommon* dummyLclVarTree = nullptr; - if (!tree->DefinesLocal(this, &dummyLclVarTree)) + if (tree->DefinesLocal(this, &dummyLclVarTree)) { - // If it doesn't define a local, then it might update the GC heap. - fgCurMemoryDef |= memoryKindSet(GcHeap); + if (lvaVarAddrExposed(dummyLclVarTree->gtLclNum)) + { + fgCurMemoryDef |= memoryKindSet(ByrefExposed); + + // We've found a store that modifies ByrefExposed + // memory but not GcHeap memory, so track their + // states separately. + byrefStatesMatchGcHeapStates = false; + } + } + else + { + // If it doesn't define a local, then it might update GcHeap/ByrefExposed. + fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed); } } break; @@ -426,6 +466,11 @@ void Compiler::fgPerBlockLocalVarLiveness() break; } } + + // In minopts, we don't explicitly build SSA or value-number; GcHeap and + // ByrefExposed implicitly (conservatively) change state at each instr. + byrefStatesMatchGcHeapStates = true; + return; } @@ -435,6 +480,10 @@ void Compiler::fgPerBlockLocalVarLiveness() VarSetOps::AssignNoCopy(this, fgCurUseSet, VarSetOps::MakeEmpty(this)); VarSetOps::AssignNoCopy(this, fgCurDefSet, VarSetOps::MakeEmpty(this)); + // GC Heap and ByrefExposed can share states unless we see a def of byref-exposed + // memory that is not a GC Heap def. + byrefStatesMatchGcHeapStates = true; + for (block = fgFirstBB; block; block = block->bbNext) { VarSetOps::ClearD(this, fgCurUseSet); @@ -537,6 +586,14 @@ void Compiler::fgPerBlockLocalVarLiveness() VarSetOps::AssignNoCopy(this, block->bbLiveIn, VarSetOps::MakeEmpty(this)); block->bbMemoryLiveIn = emptyMemoryKindSet; } + +#ifdef DEBUG + if (verbose) + { + printf("** Memory liveness computed, GcHeap states and ByrefExposed states %s\n", + (byrefStatesMatchGcHeapStates ? "match" : "diverge")); + } +#endif // DEBUG } // Helper functions to mark variables live over their entire scope diff --git a/src/coreclr/src/jit/optimizer.cpp b/src/coreclr/src/jit/optimizer.cpp index 7fce274..92edf62 100644 --- a/src/coreclr/src/jit/optimizer.cpp +++ b/src/coreclr/src/jit/optimizer.cpp @@ -6888,7 +6888,7 @@ void Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) if ((tree->gtFlags & GTF_IND_VOLATILE) != 0) { - memoryHavoc |= memoryKindSet(GcHeap); + memoryHavoc |= memoryKindSet(GcHeap, ByrefExposed); continue; } @@ -6910,12 +6910,14 @@ void Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) CORINFO_CLASS_HANDLE elemType = CORINFO_CLASS_HANDLE(vnStore->ConstantValue(funcApp.m_args[0])); AddModifiedElemTypeAllContainingLoops(mostNestedLoop, elemType); - // Don't set memoryHavoc below. + // Don't set memoryHavoc for GcHeap below. Do set memoryHavoc for ByrefExposed + // (conservatively assuming that a byref may alias the array element) + memoryHavoc |= memoryKindSet(ByrefExposed); continue; } } // Otherwise... - memoryHavoc |= memoryKindSet(GcHeap); + memoryHavoc |= memoryKindSet(GcHeap, ByrefExposed); } // Is the LHS an array index expression? else if (lhs->ParseArrayElemForm(this, &arrInfo, &fldSeqArrElem)) @@ -6924,6 +6926,8 @@ void Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) // field of "S", will lose all information about the array type. CORINFO_CLASS_HANDLE elemTypeEq = EncodeElemType(arrInfo.m_elemType, arrInfo.m_elemStructType); AddModifiedElemTypeAllContainingLoops(mostNestedLoop, elemTypeEq); + // Conservatively assume byrefs may alias this array element + memoryHavoc |= memoryKindSet(ByrefExposed); } else { @@ -6945,10 +6949,12 @@ void Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) } AddModifiedFieldAllContainingLoops(mostNestedLoop, fldSeq->m_fieldHnd); + // Conservatively assume byrefs may alias this object. + memoryHavoc |= memoryKindSet(ByrefExposed); } else { - memoryHavoc |= memoryKindSet(GcHeap); + memoryHavoc |= memoryKindSet(GcHeap, ByrefExposed); } } } @@ -6958,13 +6964,19 @@ void Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) bool isEntire; if (!tree->DefinesLocal(this, &lclVarTree, &isEntire)) { - // For now, assume arbitrary side effects on GcHeap... - memoryHavoc |= memoryKindSet(GcHeap); + // For now, assume arbitrary side effects on GcHeap/ByrefExposed... + memoryHavoc |= memoryKindSet(GcHeap, ByrefExposed); + } + else if (lvaVarAddrExposed(lclVarTree->gtLclNum)) + { + memoryHavoc |= memoryKindSet(ByrefExposed); } } else if (lhs->OperGet() == GT_CLS_VAR) { AddModifiedFieldAllContainingLoops(mostNestedLoop, lhs->gtClsVar.gtClsVarHnd); + // Conservatively assume byrefs may alias this static field + memoryHavoc |= memoryKindSet(ByrefExposed); } // Otherwise, must be local lhs form. I should assert that. else if (lhs->OperGet() == GT_LCL_VAR) @@ -6983,6 +6995,11 @@ void Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) ->m_vnPair.SetLiberal(rhsVN); } } + // If the local is address-exposed, count this as ByrefExposed havoc + if (lvaVarAddrExposed(lhsLcl->gtLclNum)) + { + memoryHavoc |= memoryKindSet(ByrefExposed); + } } } else // not GenTree::OperIsAssignment(oper) @@ -7023,7 +7040,7 @@ void Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) case GT_XCHG: // Binop case GT_CMPXCHG: // Specialop { - memoryHavoc |= memoryKindSet(GcHeap); + memoryHavoc |= memoryKindSet(GcHeap, ByrefExposed); } break; @@ -7039,7 +7056,7 @@ void Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) CorInfoHelpFunc helpFunc = eeGetHelperNum(call->gtCallMethHnd); if (s_helperCallProperties.MutatesHeap(helpFunc)) { - memoryHavoc |= memoryKindSet(GcHeap); + memoryHavoc |= memoryKindSet(GcHeap, ByrefExposed); } else if (s_helperCallProperties.MayRunCctor(helpFunc)) { @@ -7049,13 +7066,13 @@ void Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) // and might have arbitrary side effects. if ((tree->gtFlags & GTF_CALL_HOISTABLE) == 0) { - memoryHavoc |= memoryKindSet(GcHeap); + memoryHavoc |= memoryKindSet(GcHeap, ByrefExposed); } } } else { - memoryHavoc |= memoryKindSet(GcHeap); + memoryHavoc |= memoryKindSet(GcHeap, ByrefExposed); } break; } diff --git a/src/coreclr/src/jit/ssabuilder.cpp b/src/coreclr/src/jit/ssabuilder.cpp index bc7f8f2..3d74234 100644 --- a/src/coreclr/src/jit/ssabuilder.cpp +++ b/src/coreclr/src/jit/ssabuilder.cpp @@ -823,6 +823,20 @@ void SsaBuilder::InsertPhiFunctions(BasicBlock** postOrder, int count) for (MemoryKind memoryKind : allMemoryKinds()) { + if ((memoryKind == GcHeap) && m_pCompiler->byrefStatesMatchGcHeapStates) + { + // Share the PhiFunc with ByrefExposed. + assert(memoryKind > ByrefExposed); + bbInDomFront->bbMemorySsaPhiFunc[memoryKind] = bbInDomFront->bbMemorySsaPhiFunc[ByrefExposed]; + continue; + } + + // Check if this memoryKind is defined in this block. + if ((block->bbMemoryDef & memoryKindSet(memoryKind)) == 0) + { + continue; + } + // Check if memoryKind is live into block "*iterBlk". if ((bbInDomFront->bbMemoryLiveIn & memoryKindSet(memoryKind)) == 0) { @@ -964,23 +978,55 @@ void SsaBuilder::TreeRenameVariables(GenTree* tree, BasicBlock* block, SsaRename if (m_pCompiler->ehBlockHasExnFlowDsc(block)) { GenTreeLclVarCommon* lclVarNode; - if (!tree->DefinesLocal(m_pCompiler, &lclVarNode)) + + bool isLocal = tree->DefinesLocal(m_pCompiler, &lclVarNode); + bool isAddrExposedLocal = isLocal && m_pCompiler->lvaVarAddrExposed(lclVarNode->gtLclNum); + bool hasByrefHavoc = ((block->bbMemoryHavoc & memoryKindSet(ByrefExposed)) != 0); + if (!isLocal || (isAddrExposedLocal && !hasByrefHavoc)) { - // It *may* define the GC heap in a non-havoc way. Make a new SSA # -- associate with this node. + // It *may* define byref memory in a non-havoc way. Make a new SSA # -- associate with this node. unsigned count = pRenameState->CountForMemoryDef(); - pRenameState->PushMemory(GcHeap, block, count); - m_pCompiler->GetMemorySsaMap(GcHeap)->Set(tree, count); -#ifdef DEBUG - if (JitTls::GetCompiler()->verboseSsa) + if (!hasByrefHavoc) { - printf("Node "); - Compiler::printTreeID(tree); - printf(" (in try block) may define GC heap; ssa # = %d.\n", count); - } + pRenameState->PushMemory(ByrefExposed, block, count); + m_pCompiler->GetMemorySsaMap(ByrefExposed)->Set(tree, count); +#ifdef DEBUG + if (JitTls::GetCompiler()->verboseSsa) + { + printf("Node "); + Compiler::printTreeID(tree); + printf(" (in try block) may define memory; ssa # = %d.\n", count); + } #endif // DEBUG - // Now add this SSA # to all phis of the reachable catch blocks. - AddMemoryDefToHandlerPhis(GcHeap, block, count); + // Now add this SSA # to all phis of the reachable catch blocks. + AddMemoryDefToHandlerPhis(ByrefExposed, block, count); + } + + if (!isLocal) + { + // Add a new def for GcHeap as well + if (m_pCompiler->byrefStatesMatchGcHeapStates) + { + // GcHeap and ByrefExposed share the same stacks, SsaMap, and phis + assert(!hasByrefHavoc); + assert(pRenameState->CountForMemoryUse(GcHeap) == count); + assert(*m_pCompiler->GetMemorySsaMap(GcHeap)->LookupPointer(tree) == count); + assert(block->bbMemorySsaPhiFunc[GcHeap] == block->bbMemorySsaPhiFunc[ByrefExposed]); + } + else + { + if (!hasByrefHavoc) + { + // Allocate a distinct defnum for the GC Heap + count = pRenameState->CountForMemoryDef(); + } + + pRenameState->PushMemory(GcHeap, block, count); + m_pCompiler->GetMemorySsaMap(GcHeap)->Set(tree, count); + AddMemoryDefToHandlerPhis(GcHeap, block, count); + } + } } } } @@ -1191,6 +1237,21 @@ void SsaBuilder::AddMemoryDefToHandlerPhis(MemoryKind memoryKind, BasicBlock* bl // Add "count" to the phi args of memoryKind. BasicBlock::MemoryPhiArg*& handlerMemoryPhi = handler->bbMemorySsaPhiFunc[memoryKind]; + +#if DEBUG + if (m_pCompiler->byrefStatesMatchGcHeapStates) + { + // When sharing phis for GcHeap and ByrefExposed, callers should ask to add phis + // for ByrefExposed only. + assert(memoryKind != GcHeap); + if (memoryKind == ByrefExposed) + { + // The GcHeap and ByrefExposed phi funcs should always be in sync. + assert(handlerMemoryPhi == handler->bbMemorySsaPhiFunc[GcHeap]); + } + } +#endif + if (handlerMemoryPhi == BasicBlock::EmptyMemoryPhiDef) { handlerMemoryPhi = new (m_pCompiler) BasicBlock::MemoryPhiArg(count); @@ -1210,6 +1271,12 @@ void SsaBuilder::AddMemoryDefToHandlerPhis(MemoryKind memoryKind, BasicBlock* bl DBG_SSA_JITDUMP(" Added phi arg u:%d for %s to phi defn in handler block BB%02u.\n", count, memoryKindNames[memoryKind], memoryKind, handler->bbNum); + + if ((memoryKind == ByrefExposed) && m_pCompiler->byrefStatesMatchGcHeapStates) + { + // Share the phi between GcHeap and ByrefExposed. + handler->bbMemorySsaPhiFunc[GcHeap] = handlerMemoryPhi; + } } unsigned tryInd = tryBlk->ebdEnclosingTryIndex; if (tryInd == EHblkDsc::NO_ENCLOSING_INDEX) @@ -1236,14 +1303,25 @@ void SsaBuilder::BlockRenameVariables(BasicBlock* block, SsaRenameState* pRename // First handle the incoming memory states. for (MemoryKind memoryKind : allMemoryKinds()) { - // Is there an Phi definition for memoryKind at the start of this block? - if (block->bbMemorySsaPhiFunc[memoryKind] != nullptr) + if ((memoryKind == GcHeap) && m_pCompiler->byrefStatesMatchGcHeapStates) + { + // ByrefExposed and GcHeap share any phi this block may have, + assert(block->bbMemorySsaPhiFunc[memoryKind] == block->bbMemorySsaPhiFunc[ByrefExposed]); + // so we will have already allocated a defnum for it if needed. + assert(memoryKind > ByrefExposed); + assert(pRenameState->CountForMemoryUse(memoryKind) == pRenameState->CountForMemoryUse(ByrefExposed)); + } + else { - unsigned count = pRenameState->CountForMemoryDef(); - pRenameState->PushMemory(memoryKind, block, count); + // Is there an Phi definition for memoryKind at the start of this block? + if (block->bbMemorySsaPhiFunc[memoryKind] != nullptr) + { + unsigned count = pRenameState->CountForMemoryDef(); + pRenameState->PushMemory(memoryKind, block, count); - DBG_SSA_JITDUMP("Ssa # for %s phi on entry to BB%02u is %d.\n", memoryKindNames[memoryKind], block->bbNum, - count); + DBG_SSA_JITDUMP("Ssa # for %s phi on entry to BB%02u is %d.\n", memoryKindNames[memoryKind], + block->bbNum, count); + } } // Record the "in" Ssa # for memoryKind. @@ -1275,11 +1353,23 @@ void SsaBuilder::BlockRenameVariables(BasicBlock* block, SsaRenameState* pRename // If the block defines memory, allocate an SSA variable for the final memory state in the block. // (This may be redundant with the last SSA var explicitly created, but there's no harm in that.) - if ((block->bbMemoryDef & memorySet) != 0) + if ((memoryKind == GcHeap) && m_pCompiler->byrefStatesMatchGcHeapStates) { - unsigned count = pRenameState->CountForMemoryDef(); - pRenameState->PushMemory(memoryKind, block, count); - AddMemoryDefToHandlerPhis(memoryKind, block, count); + // We've already allocated the SSA num and propagated it to shared phis, if needed, + // when processing ByrefExposed. + assert(memoryKind > ByrefExposed); + assert(((block->bbMemoryDef & memorySet) != 0) == + ((block->bbMemoryDef & memoryKindSet(ByrefExposed)) != 0)); + assert(pRenameState->CountForMemoryUse(memoryKind) == pRenameState->CountForMemoryUse(ByrefExposed)); + } + else + { + if ((block->bbMemoryDef & memorySet) != 0) + { + unsigned count = pRenameState->CountForMemoryDef(); + pRenameState->PushMemory(memoryKind, block, count); + AddMemoryDefToHandlerPhis(memoryKind, block, count); + } } // Record the "out" Ssa" # for memoryKind. @@ -1353,6 +1443,20 @@ void SsaBuilder::AssignPhiNodeRhsVariables(BasicBlock* block, SsaRenameState* pR BasicBlock::MemoryPhiArg*& succMemoryPhi = succ->bbMemorySsaPhiFunc[memoryKind]; if (succMemoryPhi != nullptr) { + if ((memoryKind == GcHeap) && m_pCompiler->byrefStatesMatchGcHeapStates) + { + // We've already propagated the "out" number to the phi shared with ByrefExposed, + // but still need to update bbMemorySsaPhiFunc to be in sync between GcHeap and ByrefExposed. + assert(memoryKind > ByrefExposed); + assert(block->bbMemorySsaNumOut[memoryKind] == block->bbMemorySsaNumOut[ByrefExposed]); + assert((succ->bbMemorySsaPhiFunc[ByrefExposed] == succMemoryPhi) || + (succ->bbMemorySsaPhiFunc[ByrefExposed]->m_nextArg == + (succMemoryPhi == BasicBlock::EmptyMemoryPhiDef ? nullptr : succMemoryPhi))); + succMemoryPhi = succ->bbMemorySsaPhiFunc[ByrefExposed]; + + continue; + } + if (succMemoryPhi == BasicBlock::EmptyMemoryPhiDef) { succMemoryPhi = new (m_pCompiler) BasicBlock::MemoryPhiArg(block->bbMemorySsaNumOut[memoryKind]); @@ -1492,6 +1596,19 @@ void SsaBuilder::AssignPhiNodeRhsVariables(BasicBlock* block, SsaRenameState* pR BasicBlock::MemoryPhiArg*& handlerMemoryPhi = handlerStart->bbMemorySsaPhiFunc[memoryKind]; if (handlerMemoryPhi != nullptr) { + if ((memoryKind == GcHeap) && m_pCompiler->byrefStatesMatchGcHeapStates) + { + // We've already added the arg to the phi shared with ByrefExposed if needed, + // but still need to update bbMemorySsaPhiFunc to stay in sync. + assert(memoryKind > ByrefExposed); + assert(block->bbMemorySsaNumOut[memoryKind] == block->bbMemorySsaNumOut[ByrefExposed]); + assert(handlerStart->bbMemorySsaPhiFunc[ByrefExposed]->m_ssaNum == + block->bbMemorySsaNumOut[memoryKind]); + handlerMemoryPhi = handlerStart->bbMemorySsaPhiFunc[ByrefExposed]; + + continue; + } + if (handlerMemoryPhi == BasicBlock::EmptyMemoryPhiDef) { handlerMemoryPhi = @@ -1534,6 +1651,12 @@ void SsaBuilder::BlockPopStacks(BasicBlock* block, SsaRenameState* pRenameState) // And for memory. for (MemoryKind memoryKind : allMemoryKinds()) { + if ((memoryKind == GcHeap) && m_pCompiler->byrefStatesMatchGcHeapStates) + { + // GcHeap and ByrefExposed share a rename stack, so don't try + // to pop it a second time. + continue; + } pRenameState->PopBlockMemoryStack(memoryKind, block); } } @@ -1591,6 +1714,11 @@ void SsaBuilder::RenameVariables(BlkToBlkSetMap* domTree, SsaRenameState* pRenam assert(initMemoryCount == SsaConfig::FIRST_SSA_NUM); for (MemoryKind memoryKind : allMemoryKinds()) { + if ((memoryKind == GcHeap) && m_pCompiler->byrefStatesMatchGcHeapStates) + { + // GcHeap shares its stack with ByrefExposed; don't re-push. + continue; + } pRenameState->PushMemory(memoryKind, m_pCompiler->fgFirstBB, initMemoryCount); } @@ -1771,7 +1899,7 @@ void SsaBuilder::Build() // Rename local variables and collect UD information for each ssa var. SsaRenameState* pRenameState = new (jitstd::utility::allocate(m_allocator), jitstd::placement_t()) - SsaRenameState(m_allocator, m_pCompiler->lvaCount); + SsaRenameState(m_allocator, m_pCompiler->lvaCount, m_pCompiler->byrefStatesMatchGcHeapStates); RenameVariables(domTree, pRenameState); EndPhase(PHASE_BUILD_SSA_RENAME); diff --git a/src/coreclr/src/jit/ssarenamestate.cpp b/src/coreclr/src/jit/ssarenamestate.cpp index 07d344d..4ccac05 100644 --- a/src/coreclr/src/jit/ssarenamestate.cpp +++ b/src/coreclr/src/jit/ssarenamestate.cpp @@ -28,7 +28,9 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX * * @params alloc The allocator class used to allocate jitstd data. */ -SsaRenameState::SsaRenameState(const jitstd::allocator& alloc, unsigned lvaCount) +SsaRenameState::SsaRenameState(const jitstd::allocator& alloc, + unsigned lvaCount, + bool byrefStatesMatchGcHeapStates) : counts(nullptr) , stacks(nullptr) , definedLocs(alloc) @@ -36,6 +38,7 @@ SsaRenameState::SsaRenameState(const jitstd::allocator& alloc, unsigned lva , memoryCount(0) , lvaCount(lvaCount) , m_alloc(alloc) + , byrefStatesMatchGcHeapStates(byrefStatesMatchGcHeapStates) { } diff --git a/src/coreclr/src/jit/ssarenamestate.h b/src/coreclr/src/jit/ssarenamestate.h index 02a9d14..a8496b6 100644 --- a/src/coreclr/src/jit/ssarenamestate.h +++ b/src/coreclr/src/jit/ssarenamestate.h @@ -101,7 +101,7 @@ struct SsaRenameState typedef unsigned* Counts; typedef jitstd::list DefStack; - SsaRenameState(const jitstd::allocator& allocator, unsigned lvaCount); + SsaRenameState(const jitstd::allocator& allocator, unsigned lvaCount, bool byrefStatesMatchGcHeapStates); void EnsureCounts(); void EnsureStacks(); @@ -134,11 +134,21 @@ struct SsaRenameState } unsigned CountForMemoryUse(MemoryKind memoryKind) { + if ((memoryKind == GcHeap) && byrefStatesMatchGcHeapStates) + { + // Share rename stacks in this configuration. + memoryKind = ByrefExposed; + } return memoryStack[memoryKind].back().m_count; } void PushMemory(MemoryKind memoryKind, BasicBlock* bb, unsigned count) { + if ((memoryKind == GcHeap) && byrefStatesMatchGcHeapStates) + { + // Share rename stacks in this configuration. + memoryKind = ByrefExposed; + } memoryStack[memoryKind].push_back(SsaRenameStateForBlock(bb, count)); } @@ -173,4 +183,7 @@ private: // Allocator to allocate stacks. jitstd::allocator m_alloc; + + // Indicates whether GcHeap and ByrefExposed use the same state. + bool byrefStatesMatchGcHeapStates; }; diff --git a/src/coreclr/src/jit/valuenum.cpp b/src/coreclr/src/jit/valuenum.cpp index 37a743d..46662e8 100644 --- a/src/coreclr/src/jit/valuenum.cpp +++ b/src/coreclr/src/jit/valuenum.cpp @@ -4750,6 +4750,16 @@ ValueNum Compiler::fgMemoryVNForLoopSideEffects(MemoryKind memoryKind, } } } + else + { + // If there were any fields/elements modified, this should have been recorded as havoc + // for ByrefExposed. + assert(memoryKind == ByrefExposed); + assert((optLoopTable[loopNum].lpFieldsModified == nullptr) || + optLoopTable[loopNum].lpLoopHasMemoryHavoc[memoryKind]); + assert((optLoopTable[loopNum].lpArrayElemTypesModified == nullptr) || + optLoopTable[loopNum].lpLoopHasMemoryHavoc[memoryKind]); + } #ifdef DEBUG if (verbose)