From e86796544642bc51fa0683ec47c584c87f029b1c Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 31 May 2024 21:06:40 +0200 Subject: [PATCH] JIT: Propagate `LCL_ADDR` nodes during local morph (#102808) This changes local morph to run in RPO when optimizations are enabled. It adds infrastructure to track and propagate LCL_ADDR values assigned to locals during local morph. This allows us to avoid address exposure in cases where the destination local does not actually end up escaping in any way. Example: ```csharp public struct Awaitable { public int Opts; public Awaitable(bool value) { Opts = value ? 1 : 2; } } [MethodImpl(MethodImplOptions.NoInlining)] public static int Test() => new Awaitable(false).Opts; ``` Before: ```asm G_M59043_IG01: ;; offset=0x0000 push rax ;; size=1 bbWeight=1 PerfScore 1.00 G_M59043_IG02: ;; offset=0x0001 xor eax, eax mov dword ptr [rsp], eax mov dword ptr [rsp], 2 mov eax, dword ptr [rsp] ;; size=15 bbWeight=1 PerfScore 3.25 G_M59043_IG03: ;; offset=0x0010 add rsp, 8 ret ;; size=5 bbWeight=1 PerfScore 1.25 ; Total bytes of code: 21 ``` After: ```asm G_M59043_IG02: ;; offset=0x0000 mov eax, 2 ;; size=5 bbWeight=1 PerfScore 0.25 G_M59043_IG03: ;; offset=0x0005 ret ``` Propagating the addresses works much like local assertion prop in morph does. Proving that the locals that were stored to do not escape afterwards is done with a simplistic approach: we check globally that no reads of the locals exists, and if so, we replace the `LCL_ADDR` stored to them by a constant 0. We leave it up to liveness to clean up the stores themselves. If we were able to remove any `LCL_ADDR` in this way then we run an additional pass over the locals of the IR to compute the final set of exposed locals. This could be more sophisticated, but in practice this handles the reported cases just fine. Fix #87072 Fix #102273 Fix #102518 This is still not sufficient to handle #69254. To handle that we would need more support around tracking the values of struct fields, and handling of promoted fields. This PR currently does not handle promoted fields at all; we use `lvHasLdAddrOp` as a conservative approximation of address exposure on the destination locals, and promoted structs almost always have this set. If we were to handle promoted fields we would need some other way to determine that a destination holding a local address couldn't be exposed. --- src/coreclr/jit/compiler.h | 2 + src/coreclr/jit/jitconfigvalues.h | 1 + src/coreclr/jit/lclmorph.cpp | 686 +++++++++++++++++- src/coreclr/jit/ssabuilder.cpp | 1 + src/coreclr/jit/valuenum.cpp | 2 +- .../LocalMorphBackedge.cs | 36 + .../LocalMorphBackedge.csproj | 9 + 7 files changed, 696 insertions(+), 41 deletions(-) create mode 100644 src/tests/JIT/opt/AssertionPropagation/LocalMorphBackedge.cs create mode 100644 src/tests/JIT/opt/AssertionPropagation/LocalMorphBackedge.csproj diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index dd92e05f464af..bea5ca8507652 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6785,6 +6785,8 @@ class Compiler PhaseStatus fgMarkAddressExposedLocals(); void fgSequenceLocals(Statement* stmt); + bool fgExposeUnpropagatedLocals(bool propagatedAny, class LocalEqualsLocalAddrAssertions* assertions); + void fgExposeLocalsInBitVec(BitVec_ValArg_T bitVec); PhaseStatus PhysicalPromotion(); diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index e354e3421acbe..dbd56791d88d8 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -567,6 +567,7 @@ OPT_CONFIG_STRING(JitOnlyOptimizeRange, OPT_CONFIG_STRING(JitEnablePhysicalPromotionRange, W("JitEnablePhysicalPromotionRange")) OPT_CONFIG_STRING(JitEnableCrossBlockLocalAssertionPropRange, W("JitEnableCrossBlockLocalAssertionPropRange")) OPT_CONFIG_STRING(JitEnableInductionVariableOptsRange, W("JitEnableInductionVariableOptsRange")) +OPT_CONFIG_STRING(JitEnableLocalAddrPropagationRange, W("JitEnableLocalAddrPropagationRange")) OPT_CONFIG_INTEGER(JitDoSsa, W("JitDoSsa"), 1) // Perform Static Single Assignment (SSA) numbering on the variables OPT_CONFIG_INTEGER(JitDoValueNumber, W("JitDoValueNumber"), 1) // Perform value numbering on method expressions diff --git a/src/coreclr/jit/lclmorph.cpp b/src/coreclr/jit/lclmorph.cpp index 6a4534fc3f63a..b8ecc8b1f736f 100644 --- a/src/coreclr/jit/lclmorph.cpp +++ b/src/coreclr/jit/lclmorph.cpp @@ -175,6 +175,277 @@ void Compiler::fgSequenceLocals(Statement* stmt) seq.Sequence(stmt); } +struct LocalEqualsLocalAddrAssertion +{ + // Local num on the LHS + unsigned DestLclNum; + // Local num on the RHS (having its adress taken) + unsigned AddressLclNum; + // Offset into RHS local + unsigned AddressOffset; + + LocalEqualsLocalAddrAssertion(unsigned destLclNum, unsigned addressLclNum, unsigned addressOffset) + : DestLclNum(destLclNum) + , AddressLclNum(addressLclNum) + , AddressOffset(addressOffset) + { + } + +#ifdef DEBUG + void Print() const + { + printf("V%02u = &V%02u[+%03u]", DestLclNum, AddressLclNum, AddressOffset); + } +#endif +}; + +struct AssertionKeyFuncs +{ + static bool Equals(const LocalEqualsLocalAddrAssertion& lhs, const LocalEqualsLocalAddrAssertion rhs) + { + return (lhs.DestLclNum == rhs.DestLclNum) && (lhs.AddressLclNum == rhs.AddressLclNum) && + (lhs.AddressOffset == rhs.AddressOffset); + } + + static unsigned GetHashCode(const LocalEqualsLocalAddrAssertion& val) + { + unsigned hash = val.DestLclNum; + hash ^= val.AddressLclNum + 0x9e3779b9 + (hash << 19) + (hash >> 13); + hash ^= val.AddressOffset + 0x9e3779b9 + (hash << 19) + (hash >> 13); + return hash; + } +}; + +typedef JitHashTable AssertionToIndexMap; + +class LocalEqualsLocalAddrAssertions +{ + Compiler* m_comp; + ArrayStack m_assertions; + AssertionToIndexMap m_map; + uint64_t* m_lclAssertions; + uint64_t* m_outgoingAssertions; + uint64_t m_currentAssertions = 0; + BitVec m_localsToExpose; + +public: + LocalEqualsLocalAddrAssertions(Compiler* comp) + : m_comp(comp) + , m_assertions(comp->getAllocator(CMK_LocalAddressVisitor)) + , m_map(comp->getAllocator(CMK_LocalAddressVisitor)) + { + m_lclAssertions = + comp->lvaCount == 0 ? nullptr : new (comp, CMK_LocalAddressVisitor) uint64_t[comp->lvaCount]{}; + m_outgoingAssertions = new (comp, CMK_LocalAddressVisitor) uint64_t[comp->m_dfsTree->GetPostOrderCount()]{}; + + BitVecTraits localsTraits(comp->lvaCount, comp); + m_localsToExpose = BitVecOps::MakeEmpty(&localsTraits); + } + + //------------------------------------------------------------------------ + // GetLocalsToExpose: Get the bit vector of locals that were marked to be + // exposed. + // + BitVec_ValRet_T GetLocalsToExpose() + { + return m_localsToExpose; + } + + //------------------------------------------------------------------------ + // IsMarkedForExposure: Check if a specific local is marked to be exposed. + // + // Return Value: + // True if so. + // + bool IsMarkedForExposure(unsigned lclNum) + { + BitVecTraits traits(m_comp->lvaCount, m_comp); + if (BitVecOps::IsMember(&traits, m_localsToExpose, lclNum)) + { + return true; + } + + LclVarDsc* dsc = m_comp->lvaGetDesc(lclNum); + if (dsc->lvIsStructField && BitVecOps::IsMember(&traits, m_localsToExpose, dsc->lvParentLcl)) + { + return true; + } + + return false; + } + + //------------------------------------------------------------------- + // StartBlock: Start a new block by computing incoming assertions for the + // block. + // + // Arguments: + // block - The block + // + void StartBlock(BasicBlock* block) + { + if ((m_assertions.Height() == 0) || (block->bbPreds == nullptr) || m_comp->bbIsHandlerBeg(block)) + { + m_currentAssertions = 0; + return; + } + + m_currentAssertions = UINT64_MAX; + for (BasicBlock* pred : block->PredBlocks()) + { + assert(m_comp->m_dfsTree->Contains(pred)); + if (pred->bbPostorderNum <= block->bbPostorderNum) + { + m_currentAssertions = 0; + break; + } + + m_currentAssertions &= m_outgoingAssertions[pred->bbPostorderNum]; + } + +#ifdef DEBUG + if (m_currentAssertions != 0) + { + JITDUMP(FMT_BB " incoming assertions:\n", block->bbNum); + uint64_t assertions = m_currentAssertions; + do + { + uint32_t index = BitOperations::BitScanForward(assertions); + JITDUMP(" A%02u: ", index); + DBEXEC(VERBOSE, m_assertions.BottomRef((int)index).Print()); + JITDUMP("\n"); + + assertions ^= uint64_t(1) << index; + } while (assertions != 0); + } +#endif + } + + //------------------------------------------------------------------- + // EndBlock: End a block by recording its final outgoing assertions. + // + // Arguments: + // block - The block + // + void EndBlock(BasicBlock* block) + { + m_outgoingAssertions[block->bbPostorderNum] = m_currentAssertions; + } + + //------------------------------------------------------------------- + // OnExposed: Mark that a local is having its address taken. + // + // Arguments: + // lclNum - The local + // + void OnExposed(unsigned lclNum) + { + BitVecTraits localsTraits(m_comp->lvaCount, m_comp); + BitVecOps::AddElemD(&localsTraits, m_localsToExpose, lclNum); + } + + //------------------------------------------------------------------- + // Record: Record an assertion about the specified local. + // + // Arguments: + // dstLclNum - Destination local + // srcLclNum - Local having its address taken + // srcOffs - Offset into the source local of the address being taken + // + void Record(unsigned dstLclNum, unsigned srcLclNum, unsigned srcOffs) + { + LocalEqualsLocalAddrAssertion assertion(dstLclNum, srcLclNum, srcOffs); + + unsigned index; + if (m_assertions.Height() >= 64) + { + if (!m_map.Lookup(assertion, &index)) + { + JITDUMP("Out of assertion space; dropping assertion "); + DBEXEC(VERBOSE, assertion.Print()); + JITDUMP("\n"); + return; + } + } + else + { + unsigned* pIndex = m_map.LookupPointerOrAdd(assertion, UINT_MAX); + if (*pIndex == UINT_MAX) + { + index = (unsigned)m_assertions.Height(); + *pIndex = index; + m_assertions.Push(assertion); + m_lclAssertions[dstLclNum] |= uint64_t(1) << index; + + JITDUMP("Adding new assertion A%02u ", index); + DBEXEC(VERBOSE, assertion.Print()); + JITDUMP("\n"); + } + else + { + index = *pIndex; + JITDUMP("Adding existing assertion A%02u ", index); + DBEXEC(VERBOSE, assertion.Print()); + JITDUMP("\n"); + } + } + + m_currentAssertions |= uint64_t(1) << index; + } + + //------------------------------------------------------------------- + // Clear: Clear active assertions about the specified local. + // + // Arguments: + // dstLclNum - Destination local + // + void Clear(unsigned dstLclNum) + { + m_currentAssertions &= ~m_lclAssertions[dstLclNum]; + } + + //----------------------------------------------------------------------------------- + // GetCurrentAssertion: + // Get the current assertion about a local's value. + // + // Arguments: + // lclNum - The local + // + // Return Value: + // Assertion, or nullptr if there is no current assertion. + // + const LocalEqualsLocalAddrAssertion* GetCurrentAssertion(unsigned lclNum) + { + uint64_t curAssertion = m_currentAssertions & m_lclAssertions[lclNum]; + assert(genMaxOneBit(curAssertion)); + if (curAssertion == 0) + { + return nullptr; + } + + return &m_assertions.BottomRef(BitOperations::BitScanForward(curAssertion)); + } + + //----------------------------------------------------------------------------------- + // GetLocalsWithAssertions: + // Get a bit vector of all locals that have assertions about their value. + // + // Return Value: + // Bit vector of locals. + // + BitVec_ValRet_T GetLocalsWithAssertions() + { + BitVecTraits localsTraits(m_comp->lvaCount, m_comp); + BitVec result(BitVecOps::MakeEmpty(&localsTraits)); + + for (int i = 0; i < m_assertions.Height(); i++) + { + BitVecOps::AddElemD(&localsTraits, result, m_assertions.BottomRef(i).DestLclNum); + } + + return result; + } +}; + class LocalAddressVisitor final : public GenTreeVisitor { // During tree traversal every GenTree node produces a "value" that represents: @@ -259,6 +530,13 @@ class LocalAddressVisitor final : public GenTreeVisitor m_offset = lclAddr->GetLclOffs(); } + void Address(unsigned lclNum, unsigned lclOffs) + { + assert(IsUnknown()); + m_lclNum = lclNum; + m_offset = lclOffs; + } + //------------------------------------------------------------------------ // AddOffset: Produce an address value from an address value. // @@ -326,10 +604,12 @@ class LocalAddressVisitor final : public GenTreeVisitor LclFld }; - ArrayStack m_valueStack; - bool m_stmtModified; - bool m_madeChanges; - LocalSequencer* m_sequencer; + ArrayStack m_valueStack; + bool m_stmtModified = false; + bool m_madeChanges = false; + bool m_propagatedAddrs = false; + LocalSequencer* m_sequencer; + LocalEqualsLocalAddrAssertions* m_lclAddrAssertions; public: enum @@ -339,12 +619,11 @@ class LocalAddressVisitor final : public GenTreeVisitor UseExecutionOrder = true, }; - LocalAddressVisitor(Compiler* comp, LocalSequencer* sequencer) + LocalAddressVisitor(Compiler* comp, LocalSequencer* sequencer, LocalEqualsLocalAddrAssertions* assertions) : GenTreeVisitor(comp) , m_valueStack(comp->getAllocator(CMK_LocalAddressVisitor)) - , m_stmtModified(false) - , m_madeChanges(false) , m_sequencer(sequencer) + , m_lclAddrAssertions(assertions) { } @@ -353,6 +632,11 @@ class LocalAddressVisitor final : public GenTreeVisitor return m_madeChanges; } + bool PropagatedAnyAddresses() const + { + return m_propagatedAddrs; + } + void VisitStmt(Statement* stmt) { #ifdef DEBUG @@ -407,6 +691,46 @@ class LocalAddressVisitor final : public GenTreeVisitor #endif // DEBUG } + void VisitBlock(BasicBlock* block) + { + // Make the current basic block address available globally + m_compiler->compCurBB = block; + + if (m_lclAddrAssertions != nullptr) + { + m_lclAddrAssertions->StartBlock(block); + } + + for (Statement* const stmt : block->Statements()) + { +#ifdef FEATURE_SIMD + if (m_compiler->opts.OptimizationEnabled() && stmt->GetRootNode()->TypeIs(TYP_FLOAT) && + stmt->GetRootNode()->OperIsStore()) + { + m_madeChanges |= m_compiler->fgMorphCombineSIMDFieldStores(block, stmt); + } +#endif + + VisitStmt(stmt); + } + + // We could check for GT_JMP inside the visitor, but this node is very + // rare so keeping it here avoids pessimizing the hot code. + if (block->endsWithJmpMethod(m_compiler)) + { + // GT_JMP has implicit uses of all arguments. + for (unsigned lclNum = 0; lclNum < m_compiler->info.compArgsCount; lclNum++) + { + UpdateEarlyRefCount(lclNum, nullptr, nullptr); + } + } + + if (m_lclAddrAssertions != nullptr) + { + m_lclAddrAssertions->EndBlock(block); + } + } + // Morph promoted struct fields and count local occurrences. // // Also create and push the value produced by the visited node. This is done here @@ -499,12 +823,29 @@ class LocalAddressVisitor final : public GenTreeVisitor FALLTHROUGH; case GT_STORE_LCL_VAR: + { assert(TopValue(0).Node() == node->AsLclVarCommon()->Data()); + if (m_lclAddrAssertions != nullptr) + { + HandleLocalStoreAssertions(node->AsLclVarCommon(), TopValue(0)); + } + EscapeValue(TopValue(0), node); PopValue(); - FALLTHROUGH; + + SequenceLocal(node->AsLclVarCommon()); + break; + } case GT_LCL_VAR: + if (m_lclAddrAssertions != nullptr) + { + HandleLocalAssertions(node->AsLclVarCommon(), TopValue(0)); + } + + SequenceLocal(node->AsLclVarCommon()); + break; + case GT_LCL_FLD: SequenceLocal(node->AsLclVarCommon()); break; @@ -589,10 +930,34 @@ class LocalAddressVisitor final : public GenTreeVisitor case GT_STOREIND: case GT_STORE_BLK: + { + assert(TopValue(2).Node() == node); + assert(TopValue(1).Node() == node->AsIndir()->Addr()); assert(TopValue(0).Node() == node->AsIndir()->Data()); + + // Data value always escapes. EscapeValue(TopValue(0), node); + + if (node->AsIndir()->IsVolatile() || !TopValue(1).IsAddress()) + { + // Volatile indirections must not be removed so the address, if any, must be escaped. + EscapeValue(TopValue(1), node); + } + else + { + // This consumes the address. + ProcessIndirection(use, TopValue(1), user); + + if ((m_lclAddrAssertions != nullptr) && (*use)->OperIsLocalStore()) + { + HandleLocalStoreAssertions((*use)->AsLclVarCommon(), TopValue(0)); + } + } + PopValue(); - FALLTHROUGH; + PopValue(); + break; + } case GT_BLK: case GT_IND: @@ -607,6 +972,11 @@ class LocalAddressVisitor final : public GenTreeVisitor else { ProcessIndirection(use, TopValue(0), user); + + if ((m_lclAddrAssertions != nullptr) && (*use)->OperIs(GT_LCL_VAR)) + { + HandleLocalAssertions((*use)->AsLclVarCommon(), TopValue(1)); + } } PopValue(); @@ -754,8 +1124,16 @@ class LocalAddressVisitor final : public GenTreeVisitor if (!hasHiddenStructArg) { - m_compiler->lvaSetVarAddrExposed( - varDsc->lvIsStructField ? varDsc->lvParentLcl : lclNum DEBUGARG(AddressExposedReason::ESCAPE_ADDRESS)); + unsigned exposedLclNum = varDsc->lvIsStructField ? varDsc->lvParentLcl : lclNum; + + if (m_lclAddrAssertions != nullptr) + { + m_lclAddrAssertions->OnExposed(exposedLclNum); + } + else + { + m_compiler->lvaSetVarAddrExposed(exposedLclNum DEBUGARG(AddressExposedReason::ESCAPE_ADDRESS)); + } } #ifdef TARGET_64BIT @@ -831,8 +1209,15 @@ class LocalAddressVisitor final : public GenTreeVisitor if (isWide) { - m_compiler->lvaSetVarAddrExposed( - varDsc->lvIsStructField ? varDsc->lvParentLcl : lclNum DEBUGARG(AddressExposedReason::WIDE_INDIR)); + unsigned exposedLclNum = varDsc->lvIsStructField ? varDsc->lvParentLcl : lclNum; + if (m_lclAddrAssertions != nullptr) + { + m_lclAddrAssertions->OnExposed(exposedLclNum); + } + else + { + m_compiler->lvaSetVarAddrExposed(exposedLclNum DEBUGARG(AddressExposedReason::WIDE_INDIR)); + } MorphLocalAddress(node->AsIndir()->Addr(), lclNum, offset); node->gtFlags |= GTF_GLOB_REF; // GLOB_REF may not be set already in the "large offset" case. @@ -857,7 +1242,9 @@ class LocalAddressVisitor final : public GenTreeVisitor void MorphLocalAddress(GenTree* addr, unsigned lclNum, unsigned offset) { assert(addr->TypeIs(TYP_BYREF, TYP_I_IMPL)); - assert(m_compiler->lvaVarAddrExposed(lclNum) || m_compiler->lvaGetDesc(lclNum)->IsHiddenBufferStructArg()); + assert(m_compiler->lvaVarAddrExposed(lclNum) || + ((m_lclAddrAssertions != nullptr) && m_lclAddrAssertions->IsMarkedForExposure(lclNum)) || + m_compiler->lvaGetDesc(lclNum)->IsHiddenBufferStructArg()); if (m_compiler->IsValidLclAddr(lclNum, offset)) { @@ -1408,6 +1795,59 @@ class LocalAddressVisitor final : public GenTreeVisitor } } + //----------------------------------------------------------------------------------- + // HandleLocalStoreAssertions: + // Handle clearing and generating assertions for a local store with the + // specified data value. + // + // Argument: + // store - The local store + // data - Value representing data + // + void HandleLocalStoreAssertions(GenTreeLclVarCommon* store, Value& data) + { + m_lclAddrAssertions->Clear(store->GetLclNum()); + + if (data.IsAddress() && store->OperIs(GT_STORE_LCL_VAR)) + { + LclVarDsc* dsc = m_compiler->lvaGetDesc(store); + // TODO-CQ: We currently don't handle promoted fields, but that has + // no impact since practically all promoted structs end up with + // lvHasLdAddrOp set. + if (!dsc->lvPromoted && !dsc->lvIsStructField && !dsc->lvHasLdAddrOp) + { + m_lclAddrAssertions->Record(store->GetLclNum(), data.LclNum(), data.Offset()); + } + } + } + + //----------------------------------------------------------------------------------- + // HandleLocalAssertions: + // Try to refine the specified "addr" value based on assertions about a specified local. + // + // Argument: + // lcl - The local node + // value - Value of local; will be modified to be an address if an assertion could be found + // + void HandleLocalAssertions(GenTreeLclVarCommon* lcl, Value& value) + { + assert(lcl->OperIs(GT_LCL_VAR)); + if (!lcl->TypeIs(TYP_I_IMPL, TYP_BYREF)) + { + return; + } + + const LocalEqualsLocalAddrAssertion* assertion = m_lclAddrAssertions->GetCurrentAssertion(lcl->GetLclNum()); + if (assertion != nullptr) + { + JITDUMP("Using assertion "); + DBEXEC(VERBOSE, assertion->Print()); + JITDUMP("\n"); + value.Address(assertion->AddressLclNum, assertion->AddressOffset); + m_propagatedAddrs = true; + } + } + public: //------------------------------------------------------------------------ // UpdateEarlyRefCount: updates the ref count for locals @@ -1499,41 +1939,43 @@ class LocalAddressVisitor final : public GenTreeVisitor // PhaseStatus Compiler::fgMarkAddressExposedLocals() { - bool madeChanges = false; - LocalSequencer sequencer(this); - LocalAddressVisitor visitor(this, opts.OptimizationEnabled() ? &sequencer : nullptr); + bool madeChanges = false; - for (BasicBlock* const block : Blocks()) + if (opts.OptimizationDisabled()) { - // Make the current basic block address available globally - compCurBB = block; - - for (Statement* const stmt : block->Statements()) + LocalAddressVisitor visitor(this, nullptr, nullptr); + for (BasicBlock* const block : Blocks()) { -#ifdef FEATURE_SIMD - if (opts.OptimizationEnabled() && stmt->GetRootNode()->TypeIs(TYP_FLOAT) && - stmt->GetRootNode()->OperIsStore()) - { - madeChanges |= fgMorphCombineSIMDFieldStores(block, stmt); - } -#endif + visitor.VisitBlock(block); + } + + madeChanges = visitor.MadeChanges(); + } + else + { + LocalEqualsLocalAddrAssertions assertions(this); + LocalEqualsLocalAddrAssertions* pAssertions = &assertions; - visitor.VisitStmt(stmt); +#ifdef DEBUG + static ConfigMethodRange s_range; + s_range.EnsureInit(JitConfig.JitEnableLocalAddrPropagationRange()); + if (!s_range.Contains(info.compMethodHash())) + { + pAssertions = nullptr; } +#endif - // We could check for GT_JMP inside the visitor, but this node is very - // rare so keeping it here avoids pessimizing the hot code. - if (block->endsWithJmpMethod(this)) + LocalSequencer sequencer(this); + LocalAddressVisitor visitor(this, &sequencer, pAssertions); + for (unsigned i = m_dfsTree->GetPostOrderCount(); i != 0; i--) { - // GT_JMP has implicit uses of all arguments. - for (unsigned lclNum = 0; lclNum < info.compArgsCount; lclNum++) - { - visitor.UpdateEarlyRefCount(lclNum, nullptr, nullptr); - } + visitor.VisitBlock(m_dfsTree->GetPostOrder(i - 1)); } - } - madeChanges |= visitor.MadeChanges(); + madeChanges = visitor.MadeChanges(); + + madeChanges |= fgExposeUnpropagatedLocals(visitor.PropagatedAnyAddresses(), &assertions); + } return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } @@ -1645,3 +2087,167 @@ bool Compiler::fgMorphCombineSIMDFieldStores(BasicBlock* block, Statement* stmt) return true; } #endif // FEATURE_SIMD + +//----------------------------------------------------------------------------------- +// fgExposeUnpropagatedLocals: +// Expose the final set of locals that were computed to have their address +// taken. Deletes LCL_ADDR nodes used as the data source of a store if the +// destination can be proven to not be read, and avoid exposing these if +// possible. +// +// Argument: +// propagatedAny - Whether any LCL_ADDR values were propagated +// assertions - Data structure tracking LCL_ADDR assertions +// +// Return Value: +// True if any changes were made to the IR; otherwise false. +// +bool Compiler::fgExposeUnpropagatedLocals(bool propagatedAny, LocalEqualsLocalAddrAssertions* assertions) +{ + if (!propagatedAny) + { + fgExposeLocalsInBitVec(assertions->GetLocalsToExpose()); + return false; + } + + BitVecTraits localsTraits(lvaCount, this); + BitVec unreadLocals = assertions->GetLocalsWithAssertions(); + + struct Store + { + Statement* Statement; + GenTreeLclVarCommon* Tree; + }; + + ArrayStack stores(getAllocator(CMK_LocalAddressVisitor)); + + for (unsigned i = m_dfsTree->GetPostOrderCount(); i != 0; i--) + { + BasicBlock* block = m_dfsTree->GetPostOrder(i - 1); + + for (Statement* stmt : block->Statements()) + { + for (GenTreeLclVarCommon* lcl : stmt->LocalsTreeList()) + { + if (!BitVecOps::IsMember(&localsTraits, unreadLocals, lcl->GetLclNum())) + { + continue; + } + + if (lcl->OperIs(GT_STORE_LCL_VAR, GT_STORE_LCL_FLD)) + { + if (lcl->TypeIs(TYP_I_IMPL, TYP_BYREF) && ((lcl->Data()->gtFlags & GTF_SIDE_EFFECT) == 0)) + { + stores.Push({stmt, lcl}); + } + } + else + { + BitVecOps::RemoveElemD(&localsTraits, unreadLocals, lcl->GetLclNum()); + } + } + } + } + + if (BitVecOps::IsEmpty(&localsTraits, unreadLocals)) + { + JITDUMP("No destinations of propagated LCL_ADDR nodes are unread\n"); + fgExposeLocalsInBitVec(assertions->GetLocalsToExpose()); + return false; + } + + bool changed = false; + for (int i = 0; i < stores.Height(); i++) + { + const Store& store = stores.BottomRef(i); + assert(store.Tree->TypeIs(TYP_I_IMPL, TYP_BYREF)); + + if (BitVecOps::IsMember(&localsTraits, unreadLocals, store.Tree->GetLclNum())) + { + JITDUMP("V%02u is unread; removing store data of [%06u]\n", store.Tree->GetLclNum(), dspTreeID(store.Tree)); + DISPTREE(store.Tree); + + store.Tree->Data()->BashToConst(0, store.Tree->Data()->TypeGet()); + fgSequenceLocals(store.Statement); + + JITDUMP("\nResult:\n"); + DISPTREE(store.Tree); + JITDUMP("\n"); + changed = true; + } + } + + if (changed) + { + // Finally compute new set of exposed locals. + BitVec exposedLocals(BitVecOps::MakeEmpty(&localsTraits)); + + for (unsigned i = m_dfsTree->GetPostOrderCount(); i != 0; i--) + { + BasicBlock* block = m_dfsTree->GetPostOrder(i - 1); + + for (Statement* stmt : block->Statements()) + { + for (GenTreeLclVarCommon* lcl : stmt->LocalsTreeList()) + { + if (!lcl->OperIs(GT_LCL_ADDR)) + { + continue; + } + + LclVarDsc* lclDsc = lvaGetDesc(lcl); + unsigned exposedLclNum = lclDsc->lvIsStructField ? lclDsc->lvParentLcl : lcl->GetLclNum(); + BitVecOps::AddElemD(&localsTraits, exposedLocals, exposedLclNum); + } + } + } + + auto dumpVars = [=, &localsTraits](BitVec_ValArg_T vec, BitVec_ValArg_T other) { + const char* sep = ""; + for (unsigned lclNum = 0; lclNum < lvaCount; lclNum++) + { + if (BitVecOps::IsMember(&localsTraits, vec, lclNum)) + { + JITDUMP("%sV%02u", sep, lclNum); + sep = " "; + } + else if (BitVecOps::IsMember(&localsTraits, other, lclNum)) + { + JITDUMP("%s ", sep); + sep = " "; + } + } + }; + + // TODO-CQ: Instead of intersecting here, we should teach the above + // logic about retbuf LCL_ADDRs not leading to exposure. This should + // allow us to assert that exposedLocals is a subset of + // assertions->GetLocalsToExpose(). + BitVecOps::IntersectionD(&localsTraits, exposedLocals, assertions->GetLocalsToExpose()); + + JITDUMP("Old exposed set: "); + dumpVars(assertions->GetLocalsToExpose(), exposedLocals); + JITDUMP("\nNew exposed set: "); + dumpVars(exposedLocals, assertions->GetLocalsToExpose()); + JITDUMP("\n"); + + fgExposeLocalsInBitVec(exposedLocals); + } + + return changed; +} + +//----------------------------------------------------------------------------------- +// fgExposeLocalsInBitVec: +// Mark all locals in the bit vector as address exposed. +// +void Compiler::fgExposeLocalsInBitVec(BitVec_ValArg_T bitVec) +{ + BitVecTraits localsTraits(lvaCount, this); + BitVecOps::Iter iter(&localsTraits, bitVec); + unsigned lclNum; + while (iter.NextElem(&lclNum)) + { + lvaSetVarAddrExposed(lclNum DEBUGARG(AddressExposedReason::ESCAPE_ADDRESS)); + } +} diff --git a/src/coreclr/jit/ssabuilder.cpp b/src/coreclr/jit/ssabuilder.cpp index 0a5229e32b8b7..a0b018b3078b4 100644 --- a/src/coreclr/jit/ssabuilder.cpp +++ b/src/coreclr/jit/ssabuilder.cpp @@ -1198,6 +1198,7 @@ void SsaBuilder::RenameVariables() assert(varDsc->lvTracked); if (varDsc->lvIsParam || m_pCompiler->info.compInitMem || varDsc->lvMustInit || + (varTypeIsGC(varDsc) && !varDsc->lvHasExplicitInit) || VarSetOps::IsMember(m_pCompiler, m_pCompiler->fgFirstBB->bbLiveIn, varDsc->lvVarIndex)) { unsigned ssaNum = varDsc->lvPerSsaData.AllocSsaNum(m_allocator); diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 909e5ab768af4..58a5285b2e350 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -10007,7 +10007,7 @@ PhaseStatus Compiler::fgValueNumber() ssaDef->m_vnPair.SetBoth(initVal); ssaDef->SetBlock(fgFirstBB); } - else if (info.compInitMem || varDsc->lvMustInit || + else if (info.compInitMem || varDsc->lvMustInit || (varTypeIsGC(varDsc) && !varDsc->lvHasExplicitInit) || VarSetOps::IsMember(this, fgFirstBB->bbLiveIn, varDsc->lvVarIndex)) { // The last clause covers the use-before-def variables (the ones that are live-in to the first block), diff --git a/src/tests/JIT/opt/AssertionPropagation/LocalMorphBackedge.cs b/src/tests/JIT/opt/AssertionPropagation/LocalMorphBackedge.cs new file mode 100644 index 0000000000000..f5b3e7898a1a3 --- /dev/null +++ b/src/tests/JIT/opt/AssertionPropagation/LocalMorphBackedge.cs @@ -0,0 +1,36 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; +using Xunit; + +public unsafe class LocalMorphBackedge +{ + [Fact] + public static int TestEntryPoint() + { + int x = 1234; + int y = 5678; + int* px; + int** ppx = null; + + for (int i = 100; i < GetUpper(); i++) + { + px = &x; + + if (ppx != null) + { + *ppx = &y; + } + + *px = i; + ppx = &px; + } + + return x; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int GetUpper() => 102; +} \ No newline at end of file diff --git a/src/tests/JIT/opt/AssertionPropagation/LocalMorphBackedge.csproj b/src/tests/JIT/opt/AssertionPropagation/LocalMorphBackedge.csproj new file mode 100644 index 0000000000000..501217e4d8689 --- /dev/null +++ b/src/tests/JIT/opt/AssertionPropagation/LocalMorphBackedge.csproj @@ -0,0 +1,9 @@ + + + None + True + + + + +