From 032d415e39e766f5640204e0a7739a51b4825819 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 14 Sep 2026 18:55:09 +0000 Subject: [PATCH 1/5] Initial plan From 3868e16696942d5ffc323c66f96452b3bed2ee86 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 14 Sep 2026 19:30:07 +0000 Subject: [PATCH 2/5] Remove JIT explicit-init optimization and dependent flags Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> --- src/coreclr/jit/codegencommon.cpp | 6 -- src/coreclr/jit/compiler.cpp | 4 - src/coreclr/jit/compiler.h | 5 -- src/coreclr/jit/compiler.hpp | 40 --------- src/coreclr/jit/gentree.h | 1 - src/coreclr/jit/liveness.cpp | 82 +++++++------------ src/coreclr/jit/optimizer.cpp | 52 +----------- src/coreclr/jit/ssabuilder.cpp | 3 +- src/coreclr/jit/valuenum.cpp | 2 +- .../JIT/opt/Structs/StructWithGC_Zeroing.cs | 23 ++++++ 10 files changed, 60 insertions(+), 158 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 5b257d36dc9e5b..b006090c11b5d0 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -3887,12 +3887,6 @@ void CodeGen::genCheckUseBlockInit() continue; } - if (varDsc->lvHasExplicitInit) - { - varDsc->lvMustInit = 0; - continue; - } - const bool isTemp = varDsc->lvIsTemp; const bool hasGCPtr = varDsc->HasGCPtr(); const bool isTracked = varDsc->lvTracked; diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index e20d08a7be1b2e..c1db69dbc92e32 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -9613,10 +9613,6 @@ JITDBGAPI void __cdecl cTreeFlags(Compiler* comp, GenTree* tree) { chars += printf("[VAR_FIELD_DEATH3]"); } - if (tree->gtFlags & GTF_VAR_EXPLICIT_INIT) - { - chars += printf("[VAR_EXPLICIT_INIT]"); - } #if defined(DEBUG) if (tree->gtDebugFlags & GTF_DEBUG_VAR_CSE_REF) { diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 5e0dbba6988618..cb7e1713a35018 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -651,10 +651,6 @@ class LclVarDsc unsigned char lvSuppressedZeroInit : 1; // local needs zero init if we transform tail call to loop - unsigned char lvHasExplicitInit : 1; // The local is explicitly initialized and doesn't need zero initialization in - // the prolog. If the local has gc pointers, there are no gc-safe points - // between the prolog and the explicit initialization. - unsigned char lvIsOSRLocal : 1; // Root method local in an OSR method. Any stack home will be on the Tier0 frame. // Initial value will be defined by Tier0. Requires special handing in prolog. @@ -7620,7 +7616,6 @@ class Compiler bool IsValidLclAddr(unsigned lclNum, unsigned offset); bool IsEntireAccess(unsigned lclNum, unsigned offset, ValueSize accessSize); bool IsWideAccess(unsigned lclNum, unsigned offset, ValueSize accessSize); - bool IsPotentialGCSafePoint(GenTree* tree) const; private: bool fgNeedReturnSpillTemp(); diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 39837983b0e77f..91c78ef9d4fb5a 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -3302,41 +3302,6 @@ inline bool Compiler::IsWideAccess(unsigned lclNum, unsigned offset, ValueSize a } } -//------------------------------------------------------------------------ -// IsPotentialGCSafePoint: Can the given tree be effectively a gc safe point? -// -// Arguments: -// tree - the tree to check -// -// Return Value: -// True if the tree can be a gc safe point -// -inline bool Compiler::IsPotentialGCSafePoint(GenTree* tree) const -{ - if (((tree->gtFlags & GTF_CALL) != 0)) - { - // if this is not a No-GC helper - if (!tree->IsHelperCall() || !s_helperCallProperties.IsNoGC(tree->AsCall()->GetHelperNum())) - { - // assume that we have a safe point. - return true; - } - } - - // TYP_STRUCT-typed stores might be converted into calls (with gc safe points) in Lower. - // This is quite a conservative fix as it's hard to prove Lower won't do it at this point. - if (tree->OperIsLocalStore()) - { - return tree->TypeIs(TYP_STRUCT); - } - if (tree->OperIs(GT_STORE_BLK)) - { - return true; - } - - return false; -} - /* XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX @@ -4233,11 +4198,6 @@ bool Compiler::fgVarNeedsExplicitZeroInit(unsigned varNum, bool bbInALoop, bool return true; } - if (varDsc->lvHasExplicitInit) - { - return true; - } - if (fgVarIsNeverZeroInitializedInProlog(varNum)) { return true; diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 60477341da53e0..4ab73cac683231 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -447,7 +447,6 @@ enum GenTreeFlags : unsigned GTF_VAR_MOREUSES = 0x00800000, // GT_LCL_VAR -- this node has additional uses, for example due to cloning GTF_VAR_CONTEXT = 0x00400000, // GT_LCL_VAR -- this node is part of a runtime lookup - GTF_VAR_EXPLICIT_INIT = 0x00200000, // GT_LCL_VAR -- this node is an "explicit init" store. Valid until rationalization. // For additional flags for GT_CALL node see GTF_CALL_M_* diff --git a/src/coreclr/jit/liveness.cpp b/src/coreclr/jit/liveness.cpp index 9198ab6c529d0e..3e2c41721c47b6 100644 --- a/src/coreclr/jit/liveness.cpp +++ b/src/coreclr/jit/liveness.cpp @@ -109,7 +109,7 @@ class Liveness void ComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALARG_TP keepAliveVars); bool IsTrackedCallDefinition(LIR::Range& range, GenTree* node); - bool TryRemoveDeadStoreLIR(GenTree* store, GenTreeLclVarCommon* lclNode, BasicBlock* block); + void RemoveDeadStoreLIR(GenTree* store, BasicBlock* block); bool TryRemoveNonLocalLIR(GenTree* node, LIR::Range* blockRange); bool CanUncontainOrRemoveOperands(GenTree* node); @@ -2359,38 +2359,36 @@ void Liveness::ComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VAR { GenTreeIndir* const store = addrUse.User()->AsIndir(); - if (TryRemoveDeadStoreLIR(store, node->AsLclVarCommon(), block)) - { + RemoveDeadStoreLIR(store, block); - JITDUMP("Removing dead LclVar address:\n"); - DISPNODE(node); - blockRange.Remove(node); + JITDUMP("Removing dead LclVar address:\n"); + DISPNODE(node); + blockRange.Remove(node); - GenTree* data = store->AsIndir()->Data(); - data->SetUnusedValue(); + GenTree* data = store->AsIndir()->Data(); + data->SetUnusedValue(); - if (data->isIndir()) - { - Lowering::TransformUnusedIndirection(data->AsIndir(), m_compiler, block); - } - else if (data->OperIs(GT_LCL_VAR, GT_LCL_FLD)) + if (data->isIndir()) + { + Lowering::TransformUnusedIndirection(data->AsIndir(), m_compiler, block); + } + else if (data->OperIs(GT_LCL_VAR, GT_LCL_FLD)) + { + // The unused lcl_var or lcl_field on the rhs of a removed block store may be a + // struct which cannot always be loaded onto the Wasm evaluation stack or into + // native registers, so we need to make sure to remove the node. In some cases the + // node is after us in the iteration order and will be automatically removed, but we + // may have already iterated over it without removing it, so it's necessary to clean + // up here. + JITDUMP("Removing dead store data:\n"); + DISPNODE(data); + if (next == data) { - // The unused lcl_var or lcl_field on the rhs of a removed block store may be a - // struct which cannot always be loaded onto the Wasm evaluation stack or into - // native registers, so we need to make sure to remove the node. In some cases the - // node is after us in the iteration order and will be automatically removed, but we - // may have already iterated over it without removing it, so it's necessary to clean - // up here. - JITDUMP("Removing dead store data:\n"); - DISPNODE(data); - if (next == data) - { - next = data->gtPrev; - } - assert(end != data); - blockRange.Delete(m_compiler, block, data); - // fgStmtRemoved was already set by TryRemoveDeadStoreLIR + next = data->gtPrev; } + assert(end != data); + blockRange.Delete(m_compiler, block, data); + // fgStmtRemoved was already set by RemoveDeadStoreLIR } } } @@ -2412,8 +2410,10 @@ void Liveness::ComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VAR isDeadStore = ComputeLifeUntrackedLocal(life, keepAliveVars, varDsc, lclVarNode); } - if (TLiveness::EliminateDeadCode && isDeadStore && TryRemoveDeadStoreLIR(node, lclVarNode, block)) + if (TLiveness::EliminateDeadCode && isDeadStore) { + RemoveDeadStoreLIR(node, block); + GenTree* value = lclVarNode->Data(); value->SetUnusedValue(); @@ -2595,40 +2595,20 @@ bool Liveness::IsTrackedCallDefinition(LIR::Range& range, GenTree* no } //--------------------------------------------------------------------- -// fgTryRemoveDeadStoreLIR - try to remove a dead store from LIR +// RemoveDeadStoreLIR - remove a dead store from LIR // // Arguments: // store - A store tree -// lclNode - The node representing the local being stored to // block - Block that the store is part of // -// Return Value: -// Whether the store was successfully removed from "block"'s range. -// template -bool Liveness::TryRemoveDeadStoreLIR(GenTree* store, GenTreeLclVarCommon* lclNode, BasicBlock* block) +void Liveness::RemoveDeadStoreLIR(GenTree* store, BasicBlock* block) { - // We cannot remove stores to (tracked) TYP_STRUCT locals with GC pointers marked as "explicit init", - // as said locals will be reported to the GC untracked, and deleting the explicit initializer risks - // exposing uninitialized references. - if ((lclNode->gtFlags & GTF_VAR_USEASG) == 0) - { - LclVarDsc* varDsc = m_compiler->lvaGetDesc(lclNode); - if (varDsc->lvHasExplicitInit && varDsc->TypeIs(TYP_STRUCT) && varDsc->HasGCPtr() && (varDsc->lvRefCnt() > 1)) - { - JITDUMP("Not removing a potential explicit init [%06u] of V%02u\n", Compiler::dspTreeID(store), - lclNode->GetLclNum()); - return false; - } - } - JITDUMP("Removing dead %s:\n", store->OperIsIndir() ? "indirect store" : "local store"); DISPNODE(store); LIR::AsRange(block).Remove(store); m_compiler->fgStmtRemoved = true; - - return true; } //--------------------------------------------------------------------- diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index c5d6728c1e785c..5d79bd2b498940 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -5738,18 +5738,8 @@ typedef JitHashTable, unsigned> Lc // basic block successor or until it detects a loop. It keeps track of local nodes it encounters. // When it gets to a store to a local variable or a local field, it checks whether the store // is the first reference to the local (or to the parent of the local field), and, if so, -// it may do one of two optimizations: -// 1. If the following conditions are true: -// the local is untracked, -// the value to store is 0, -// the local is guaranteed to be fully initialized in the prolog, -// then the explicit zero initialization is removed. -// 2. If the following conditions are true: -// the store is to a local (and not a field), -// the local is not lvLiveInOutOfHndlr or no exceptions can be thrown between the prolog and the store, -// either the local has no gc pointers or there are no gc-safe points between the prolog and the store, -// then the local is marked with lvHasExplicitInit which tells the codegen not to insert zero initialization -// for this local in the prolog. +// it may remove the explicit zero initialization if the local is guaranteed to be initialized in the prolog +// or by a dominating explicit zero initialization. // void Compiler::optRemoveRedundantZeroInits() { @@ -5763,9 +5753,7 @@ void Compiler::optRemoveRedundantZeroInits() CompAllocator allocator(getAllocator(CMK_ZeroInit)); LclVarRefCounts refCounts(allocator); BitVecTraits bitVecTraits(lvaCount, this); - BitVec zeroInitLocals = BitVecOps::MakeEmpty(&bitVecTraits); - bool hasGCSafePoint = false; - bool hasImplicitControlFlow = false; + BitVec zeroInitLocals = BitVecOps::MakeEmpty(&bitVecTraits); assert(fgNodeThreading == NodeThreading::AllTrees); @@ -5801,16 +5789,12 @@ void Compiler::optRemoveRedundantZeroInits() CompAllocator allocator(getAllocator(CMK_ZeroInit)); LclVarRefCounts defsInBlock(allocator); bool removedTrackedDefs = false; - bool hasEHSuccs = block->HasPotentialEHSuccs(this); for (Statement* stmt = block->FirstNonPhiDef(); stmt != nullptr;) { Statement* next = stmt->GetNextStmt(); for (GenTree* const tree : stmt->TreeList()) { - hasImplicitControlFlow |= hasEHSuccs && ((tree->gtFlags & GTF_EXCEPT) != 0); - hasGCSafePoint |= IsPotentialGCSafePoint(tree); - switch (tree->gtOper) { case GT_LCL_VAR: @@ -5911,8 +5895,7 @@ void Compiler::optRemoveRedundantZeroInits() } // The local hasn't been referenced before this store. - bool removedExplicitZeroInit = false; - bool isEntire = !tree->IsPartialLclFld(this); + bool isEntire = !tree->IsPartialLclFld(this); if (tree->Data()->IsIntegralConst(0)) { @@ -5937,7 +5920,6 @@ void Compiler::optRemoveRedundantZeroInits() if (tree == stmt->GetRootNode()) { fgRemoveStmt(block, stmt); - removedExplicitZeroInit = true; lclDsc->lvSuppressedZeroInit = 1; if (lclDsc->lvTracked) @@ -5957,25 +5939,6 @@ void Compiler::optRemoveRedundantZeroInits() } } - // For async methods we may skip an explicit init through the resumption path - // - if (!removedExplicitZeroInit && isEntire && !compIsAsync() && - (!hasImplicitControlFlow || (lclDsc->lvTracked && !lclDsc->IsLiveInOutOfHandler()))) - { - // If compMethodRequiresPInvokeFrame() returns true, lower may later - // insert a call to CORINFO_HELP_INIT_PINVOKE_FRAME but that is not a gc-safe point. - assert(s_helperCallProperties.IsNoGC(CORINFO_HELP_INIT_PINVOKE_FRAME)); - - if (!lclDsc->HasGCPtr() || (!GetInterruptible() && !hasGCSafePoint)) - { - // The local hasn't been used and won't be reported to the gc between - // the prolog and this explicit initialization. Therefore, it doesn't - // require zero initialization in the prolog. - lclDsc->lvHasExplicitInit = 1; - lclNode->gtFlags |= GTF_VAR_EXPLICIT_INIT; - JITDUMP("Marking V%02u as having an explicit init\n", lclNum); - } - } break; } default: @@ -6071,13 +6034,6 @@ PhaseStatus Compiler::optVNBasedDeadStoreRemoval() continue; } - if ((store->gtFlags & GTF_VAR_EXPLICIT_INIT) != 0) - { - // Removing explicit inits is not profitable for primitives and not safe for structs. - JITDUMP(" -- no; 'explicit init'\n"); - continue; - } - // CQ heuristic: avoid removing defs of enregisterable locals where this is likely to // make them "must-init", extending live ranges. Here we assume the first SSA def was // the implicit "live-in" one, which is not guaranteed, but very likely. diff --git a/src/coreclr/jit/ssabuilder.cpp b/src/coreclr/jit/ssabuilder.cpp index 91b7c0f94e97ef..f90ff8b1428233 100644 --- a/src/coreclr/jit/ssabuilder.cpp +++ b/src/coreclr/jit/ssabuilder.cpp @@ -1099,8 +1099,7 @@ void SsaBuilder::RenameVariables() LclVarDsc* varDsc = m_compiler->lvaGetDesc(lclNum); assert(varDsc->lvTracked); - if (varDsc->lvIsParam || m_compiler->info.compInitMem || varDsc->lvMustInit || - (varTypeIsGC(varDsc) && !varDsc->lvHasExplicitInit) || + if (varDsc->lvIsParam || m_compiler->info.compInitMem || varDsc->lvMustInit || varTypeIsGC(varDsc) || VarSetOps::IsMember(m_compiler, m_compiler->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 51ac48dbd8516a..b50c274784d9c0 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -11952,7 +11952,7 @@ PhaseStatus Compiler::fgValueNumber() ssaDef->m_vnPair.SetBoth(initVal); ssaDef->SetBlock(fgFirstBB); } - else if (info.compInitMem || varDsc->lvMustInit || (varTypeIsGC(varDsc) && !varDsc->lvHasExplicitInit) || + else if (info.compInitMem || varDsc->lvMustInit || varTypeIsGC(varDsc) || 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/Structs/StructWithGC_Zeroing.cs b/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs index 0a8719a2430a3b..e41ef11033706a 100644 --- a/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs +++ b/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +#nullable enable + using System; using System.Runtime.CompilerServices; using Xunit; @@ -40,6 +42,27 @@ private static void ZeroIt2(ref LargeStructWithGC2 s) s = default; } + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("hello")] + public static void ExplicitlyInitializedReferenceIsZeroedInProlog(string? value) + { + Assert.Equal(value, InitializeReference(value)); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static string? InitializeReference(string? value) + { + // X64: xor eax, eax + // X64: mov {{[qg]}}word ptr [{{.*}}], rax + string? s = value; + return ReadReference(ref s); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static string? ReadReference(ref string? s) => s; + struct LargeStructWithGC // 360 bytes (64-bit) { public string str; From 169bc3d7b33f1ad5139eef6e93dc52de60b3f1c5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 14 Sep 2026 19:37:08 +0000 Subject: [PATCH 3/5] Make GC prolog regression checks self-contained and register-independent Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> --- src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs | 5 +++-- src/tests/JIT/opt/Structs/StructWithGC_Zeroing.csproj | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs b/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs index e41ef11033706a..6b284fa8037769 100644 --- a/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs +++ b/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs @@ -54,8 +54,9 @@ public static void ExplicitlyInitializedReferenceIsZeroedInProlog(string? value) [MethodImpl(MethodImplOptions.NoInlining)] private static string? InitializeReference(string? value) { - // X64: xor eax, eax - // X64: mov {{[qg]}}word ptr [{{.*}}], rax + // X64: xor {{e|r}}[[ZERO:[a-z]{2}|[0-9]+]]{{d?}}, {{e|r}}[[ZERO]]{{d?}} + // X64-NEXT: mov {{[qg]}}word ptr [{{.*}}], r[[ZERO]] + // X64: call {{.*}}ReadReference string? s = value; return ReadReference(ref s); } diff --git a/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.csproj b/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.csproj index 7aa59749804e49..42b40492d75a71 100644 --- a/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.csproj +++ b/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.csproj @@ -9,5 +9,7 @@ true + + From 735995d86f327fdba3262fd457570978d4d6c34d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 14 Sep 2026 19:59:26 +0000 Subject: [PATCH 4/5] Revert StructWithGC_Zeroing test changes Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> --- .../JIT/opt/Structs/StructWithGC_Zeroing.cs | 24 ------------------- .../opt/Structs/StructWithGC_Zeroing.csproj | 2 -- 2 files changed, 26 deletions(-) diff --git a/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs b/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs index 6b284fa8037769..0a8719a2430a3b 100644 --- a/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs +++ b/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs @@ -1,8 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -#nullable enable - using System; using System.Runtime.CompilerServices; using Xunit; @@ -42,28 +40,6 @@ private static void ZeroIt2(ref LargeStructWithGC2 s) s = default; } - [Theory] - [InlineData(null)] - [InlineData("")] - [InlineData("hello")] - public static void ExplicitlyInitializedReferenceIsZeroedInProlog(string? value) - { - Assert.Equal(value, InitializeReference(value)); - } - - [MethodImpl(MethodImplOptions.NoInlining)] - private static string? InitializeReference(string? value) - { - // X64: xor {{e|r}}[[ZERO:[a-z]{2}|[0-9]+]]{{d?}}, {{e|r}}[[ZERO]]{{d?}} - // X64-NEXT: mov {{[qg]}}word ptr [{{.*}}], r[[ZERO]] - // X64: call {{.*}}ReadReference - string? s = value; - return ReadReference(ref s); - } - - [MethodImpl(MethodImplOptions.NoInlining)] - private static string? ReadReference(ref string? s) => s; - struct LargeStructWithGC // 360 bytes (64-bit) { public string str; diff --git a/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.csproj b/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.csproj index 42b40492d75a71..7aa59749804e49 100644 --- a/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.csproj +++ b/src/tests/JIT/opt/Structs/StructWithGC_Zeroing.csproj @@ -9,7 +9,5 @@ true - - From 9c32beb703087cc9b5304c0889e33fecce03349a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 17 Sep 2026 20:31:30 +0000 Subject: [PATCH 5/5] Changes before error encountered Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/da0dccf9-8520-43ca-9c90-bfdee6c39f9a Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> --- src/coreclr/jit/codegencommon.cpp | 6 +++ src/coreclr/jit/compiler.cpp | 4 ++ src/coreclr/jit/compiler.h | 5 ++ src/coreclr/jit/compiler.hpp | 40 +++++++++++++++ src/coreclr/jit/gentree.h | 1 + src/coreclr/jit/ifconversion.cpp | 6 +++ src/coreclr/jit/liveness.cpp | 82 +++++++++++++++++++------------ src/coreclr/jit/optimizer.cpp | 52 ++++++++++++++++++-- src/coreclr/jit/ssabuilder.cpp | 3 +- src/coreclr/jit/valuenum.cpp | 2 +- 10 files changed, 164 insertions(+), 37 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index b006090c11b5d0..5b257d36dc9e5b 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -3887,6 +3887,12 @@ void CodeGen::genCheckUseBlockInit() continue; } + if (varDsc->lvHasExplicitInit) + { + varDsc->lvMustInit = 0; + continue; + } + const bool isTemp = varDsc->lvIsTemp; const bool hasGCPtr = varDsc->HasGCPtr(); const bool isTracked = varDsc->lvTracked; diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index c1db69dbc92e32..e20d08a7be1b2e 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -9613,6 +9613,10 @@ JITDBGAPI void __cdecl cTreeFlags(Compiler* comp, GenTree* tree) { chars += printf("[VAR_FIELD_DEATH3]"); } + if (tree->gtFlags & GTF_VAR_EXPLICIT_INIT) + { + chars += printf("[VAR_EXPLICIT_INIT]"); + } #if defined(DEBUG) if (tree->gtDebugFlags & GTF_DEBUG_VAR_CSE_REF) { diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index cb7e1713a35018..5e0dbba6988618 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -651,6 +651,10 @@ class LclVarDsc unsigned char lvSuppressedZeroInit : 1; // local needs zero init if we transform tail call to loop + unsigned char lvHasExplicitInit : 1; // The local is explicitly initialized and doesn't need zero initialization in + // the prolog. If the local has gc pointers, there are no gc-safe points + // between the prolog and the explicit initialization. + unsigned char lvIsOSRLocal : 1; // Root method local in an OSR method. Any stack home will be on the Tier0 frame. // Initial value will be defined by Tier0. Requires special handing in prolog. @@ -7616,6 +7620,7 @@ class Compiler bool IsValidLclAddr(unsigned lclNum, unsigned offset); bool IsEntireAccess(unsigned lclNum, unsigned offset, ValueSize accessSize); bool IsWideAccess(unsigned lclNum, unsigned offset, ValueSize accessSize); + bool IsPotentialGCSafePoint(GenTree* tree) const; private: bool fgNeedReturnSpillTemp(); diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 91c78ef9d4fb5a..39837983b0e77f 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -3302,6 +3302,41 @@ inline bool Compiler::IsWideAccess(unsigned lclNum, unsigned offset, ValueSize a } } +//------------------------------------------------------------------------ +// IsPotentialGCSafePoint: Can the given tree be effectively a gc safe point? +// +// Arguments: +// tree - the tree to check +// +// Return Value: +// True if the tree can be a gc safe point +// +inline bool Compiler::IsPotentialGCSafePoint(GenTree* tree) const +{ + if (((tree->gtFlags & GTF_CALL) != 0)) + { + // if this is not a No-GC helper + if (!tree->IsHelperCall() || !s_helperCallProperties.IsNoGC(tree->AsCall()->GetHelperNum())) + { + // assume that we have a safe point. + return true; + } + } + + // TYP_STRUCT-typed stores might be converted into calls (with gc safe points) in Lower. + // This is quite a conservative fix as it's hard to prove Lower won't do it at this point. + if (tree->OperIsLocalStore()) + { + return tree->TypeIs(TYP_STRUCT); + } + if (tree->OperIs(GT_STORE_BLK)) + { + return true; + } + + return false; +} + /* XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX @@ -4198,6 +4233,11 @@ bool Compiler::fgVarNeedsExplicitZeroInit(unsigned varNum, bool bbInALoop, bool return true; } + if (varDsc->lvHasExplicitInit) + { + return true; + } + if (fgVarIsNeverZeroInitializedInProlog(varNum)) { return true; diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 4ab73cac683231..60477341da53e0 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -447,6 +447,7 @@ enum GenTreeFlags : unsigned GTF_VAR_MOREUSES = 0x00800000, // GT_LCL_VAR -- this node has additional uses, for example due to cloning GTF_VAR_CONTEXT = 0x00400000, // GT_LCL_VAR -- this node is part of a runtime lookup + GTF_VAR_EXPLICIT_INIT = 0x00200000, // GT_LCL_VAR -- this node is an "explicit init" store. Valid until rationalization. // For additional flags for GT_CALL node see GTF_CALL_M_* diff --git a/src/coreclr/jit/ifconversion.cpp b/src/coreclr/jit/ifconversion.cpp index 893c53137c9b16..5024c641d50aa9 100644 --- a/src/coreclr/jit/ifconversion.cpp +++ b/src/coreclr/jit/ifconversion.cpp @@ -290,6 +290,12 @@ bool OptIfConversionDsc::IfConvertTryGetElseFromJtrueBlock(GenTreeLclVar* thenSt GenTreeLclVar* prevStore = tree->AsLclVar(); if (prevStore->GetLclNum() == targetLclNum) { + // Sinking an explicit init could expose an uninitialized GC local at a safepoint. + if ((prevStore->gtFlags & GTF_VAR_EXPLICIT_INIT) != 0) + { + return false; + } + if (prevStore->Data()->IsInvariant()) { m_elseOperation.block = m_startBlock; diff --git a/src/coreclr/jit/liveness.cpp b/src/coreclr/jit/liveness.cpp index 3e2c41721c47b6..9198ab6c529d0e 100644 --- a/src/coreclr/jit/liveness.cpp +++ b/src/coreclr/jit/liveness.cpp @@ -109,7 +109,7 @@ class Liveness void ComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALARG_TP keepAliveVars); bool IsTrackedCallDefinition(LIR::Range& range, GenTree* node); - void RemoveDeadStoreLIR(GenTree* store, BasicBlock* block); + bool TryRemoveDeadStoreLIR(GenTree* store, GenTreeLclVarCommon* lclNode, BasicBlock* block); bool TryRemoveNonLocalLIR(GenTree* node, LIR::Range* blockRange); bool CanUncontainOrRemoveOperands(GenTree* node); @@ -2359,36 +2359,38 @@ void Liveness::ComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VAR { GenTreeIndir* const store = addrUse.User()->AsIndir(); - RemoveDeadStoreLIR(store, block); + if (TryRemoveDeadStoreLIR(store, node->AsLclVarCommon(), block)) + { - JITDUMP("Removing dead LclVar address:\n"); - DISPNODE(node); - blockRange.Remove(node); + JITDUMP("Removing dead LclVar address:\n"); + DISPNODE(node); + blockRange.Remove(node); - GenTree* data = store->AsIndir()->Data(); - data->SetUnusedValue(); + GenTree* data = store->AsIndir()->Data(); + data->SetUnusedValue(); - if (data->isIndir()) - { - Lowering::TransformUnusedIndirection(data->AsIndir(), m_compiler, block); - } - else if (data->OperIs(GT_LCL_VAR, GT_LCL_FLD)) - { - // The unused lcl_var or lcl_field on the rhs of a removed block store may be a - // struct which cannot always be loaded onto the Wasm evaluation stack or into - // native registers, so we need to make sure to remove the node. In some cases the - // node is after us in the iteration order and will be automatically removed, but we - // may have already iterated over it without removing it, so it's necessary to clean - // up here. - JITDUMP("Removing dead store data:\n"); - DISPNODE(data); - if (next == data) + if (data->isIndir()) { - next = data->gtPrev; + Lowering::TransformUnusedIndirection(data->AsIndir(), m_compiler, block); + } + else if (data->OperIs(GT_LCL_VAR, GT_LCL_FLD)) + { + // The unused lcl_var or lcl_field on the rhs of a removed block store may be a + // struct which cannot always be loaded onto the Wasm evaluation stack or into + // native registers, so we need to make sure to remove the node. In some cases the + // node is after us in the iteration order and will be automatically removed, but we + // may have already iterated over it without removing it, so it's necessary to clean + // up here. + JITDUMP("Removing dead store data:\n"); + DISPNODE(data); + if (next == data) + { + next = data->gtPrev; + } + assert(end != data); + blockRange.Delete(m_compiler, block, data); + // fgStmtRemoved was already set by TryRemoveDeadStoreLIR } - assert(end != data); - blockRange.Delete(m_compiler, block, data); - // fgStmtRemoved was already set by RemoveDeadStoreLIR } } } @@ -2410,10 +2412,8 @@ void Liveness::ComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VAR isDeadStore = ComputeLifeUntrackedLocal(life, keepAliveVars, varDsc, lclVarNode); } - if (TLiveness::EliminateDeadCode && isDeadStore) + if (TLiveness::EliminateDeadCode && isDeadStore && TryRemoveDeadStoreLIR(node, lclVarNode, block)) { - RemoveDeadStoreLIR(node, block); - GenTree* value = lclVarNode->Data(); value->SetUnusedValue(); @@ -2595,20 +2595,40 @@ bool Liveness::IsTrackedCallDefinition(LIR::Range& range, GenTree* no } //--------------------------------------------------------------------- -// RemoveDeadStoreLIR - remove a dead store from LIR +// fgTryRemoveDeadStoreLIR - try to remove a dead store from LIR // // Arguments: // store - A store tree +// lclNode - The node representing the local being stored to // block - Block that the store is part of // +// Return Value: +// Whether the store was successfully removed from "block"'s range. +// template -void Liveness::RemoveDeadStoreLIR(GenTree* store, BasicBlock* block) +bool Liveness::TryRemoveDeadStoreLIR(GenTree* store, GenTreeLclVarCommon* lclNode, BasicBlock* block) { + // We cannot remove stores to (tracked) TYP_STRUCT locals with GC pointers marked as "explicit init", + // as said locals will be reported to the GC untracked, and deleting the explicit initializer risks + // exposing uninitialized references. + if ((lclNode->gtFlags & GTF_VAR_USEASG) == 0) + { + LclVarDsc* varDsc = m_compiler->lvaGetDesc(lclNode); + if (varDsc->lvHasExplicitInit && varDsc->TypeIs(TYP_STRUCT) && varDsc->HasGCPtr() && (varDsc->lvRefCnt() > 1)) + { + JITDUMP("Not removing a potential explicit init [%06u] of V%02u\n", Compiler::dspTreeID(store), + lclNode->GetLclNum()); + return false; + } + } + JITDUMP("Removing dead %s:\n", store->OperIsIndir() ? "indirect store" : "local store"); DISPNODE(store); LIR::AsRange(block).Remove(store); m_compiler->fgStmtRemoved = true; + + return true; } //--------------------------------------------------------------------- diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 5d79bd2b498940..c5d6728c1e785c 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -5738,8 +5738,18 @@ typedef JitHashTable, unsigned> Lc // basic block successor or until it detects a loop. It keeps track of local nodes it encounters. // When it gets to a store to a local variable or a local field, it checks whether the store // is the first reference to the local (or to the parent of the local field), and, if so, -// it may remove the explicit zero initialization if the local is guaranteed to be initialized in the prolog -// or by a dominating explicit zero initialization. +// it may do one of two optimizations: +// 1. If the following conditions are true: +// the local is untracked, +// the value to store is 0, +// the local is guaranteed to be fully initialized in the prolog, +// then the explicit zero initialization is removed. +// 2. If the following conditions are true: +// the store is to a local (and not a field), +// the local is not lvLiveInOutOfHndlr or no exceptions can be thrown between the prolog and the store, +// either the local has no gc pointers or there are no gc-safe points between the prolog and the store, +// then the local is marked with lvHasExplicitInit which tells the codegen not to insert zero initialization +// for this local in the prolog. // void Compiler::optRemoveRedundantZeroInits() { @@ -5753,7 +5763,9 @@ void Compiler::optRemoveRedundantZeroInits() CompAllocator allocator(getAllocator(CMK_ZeroInit)); LclVarRefCounts refCounts(allocator); BitVecTraits bitVecTraits(lvaCount, this); - BitVec zeroInitLocals = BitVecOps::MakeEmpty(&bitVecTraits); + BitVec zeroInitLocals = BitVecOps::MakeEmpty(&bitVecTraits); + bool hasGCSafePoint = false; + bool hasImplicitControlFlow = false; assert(fgNodeThreading == NodeThreading::AllTrees); @@ -5789,12 +5801,16 @@ void Compiler::optRemoveRedundantZeroInits() CompAllocator allocator(getAllocator(CMK_ZeroInit)); LclVarRefCounts defsInBlock(allocator); bool removedTrackedDefs = false; + bool hasEHSuccs = block->HasPotentialEHSuccs(this); for (Statement* stmt = block->FirstNonPhiDef(); stmt != nullptr;) { Statement* next = stmt->GetNextStmt(); for (GenTree* const tree : stmt->TreeList()) { + hasImplicitControlFlow |= hasEHSuccs && ((tree->gtFlags & GTF_EXCEPT) != 0); + hasGCSafePoint |= IsPotentialGCSafePoint(tree); + switch (tree->gtOper) { case GT_LCL_VAR: @@ -5895,7 +5911,8 @@ void Compiler::optRemoveRedundantZeroInits() } // The local hasn't been referenced before this store. - bool isEntire = !tree->IsPartialLclFld(this); + bool removedExplicitZeroInit = false; + bool isEntire = !tree->IsPartialLclFld(this); if (tree->Data()->IsIntegralConst(0)) { @@ -5920,6 +5937,7 @@ void Compiler::optRemoveRedundantZeroInits() if (tree == stmt->GetRootNode()) { fgRemoveStmt(block, stmt); + removedExplicitZeroInit = true; lclDsc->lvSuppressedZeroInit = 1; if (lclDsc->lvTracked) @@ -5939,6 +5957,25 @@ void Compiler::optRemoveRedundantZeroInits() } } + // For async methods we may skip an explicit init through the resumption path + // + if (!removedExplicitZeroInit && isEntire && !compIsAsync() && + (!hasImplicitControlFlow || (lclDsc->lvTracked && !lclDsc->IsLiveInOutOfHandler()))) + { + // If compMethodRequiresPInvokeFrame() returns true, lower may later + // insert a call to CORINFO_HELP_INIT_PINVOKE_FRAME but that is not a gc-safe point. + assert(s_helperCallProperties.IsNoGC(CORINFO_HELP_INIT_PINVOKE_FRAME)); + + if (!lclDsc->HasGCPtr() || (!GetInterruptible() && !hasGCSafePoint)) + { + // The local hasn't been used and won't be reported to the gc between + // the prolog and this explicit initialization. Therefore, it doesn't + // require zero initialization in the prolog. + lclDsc->lvHasExplicitInit = 1; + lclNode->gtFlags |= GTF_VAR_EXPLICIT_INIT; + JITDUMP("Marking V%02u as having an explicit init\n", lclNum); + } + } break; } default: @@ -6034,6 +6071,13 @@ PhaseStatus Compiler::optVNBasedDeadStoreRemoval() continue; } + if ((store->gtFlags & GTF_VAR_EXPLICIT_INIT) != 0) + { + // Removing explicit inits is not profitable for primitives and not safe for structs. + JITDUMP(" -- no; 'explicit init'\n"); + continue; + } + // CQ heuristic: avoid removing defs of enregisterable locals where this is likely to // make them "must-init", extending live ranges. Here we assume the first SSA def was // the implicit "live-in" one, which is not guaranteed, but very likely. diff --git a/src/coreclr/jit/ssabuilder.cpp b/src/coreclr/jit/ssabuilder.cpp index f90ff8b1428233..91b7c0f94e97ef 100644 --- a/src/coreclr/jit/ssabuilder.cpp +++ b/src/coreclr/jit/ssabuilder.cpp @@ -1099,7 +1099,8 @@ void SsaBuilder::RenameVariables() LclVarDsc* varDsc = m_compiler->lvaGetDesc(lclNum); assert(varDsc->lvTracked); - if (varDsc->lvIsParam || m_compiler->info.compInitMem || varDsc->lvMustInit || varTypeIsGC(varDsc) || + if (varDsc->lvIsParam || m_compiler->info.compInitMem || varDsc->lvMustInit || + (varTypeIsGC(varDsc) && !varDsc->lvHasExplicitInit) || VarSetOps::IsMember(m_compiler, m_compiler->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 b50c274784d9c0..51ac48dbd8516a 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -11952,7 +11952,7 @@ PhaseStatus Compiler::fgValueNumber() ssaDef->m_vnPair.SetBoth(initVal); ssaDef->SetBlock(fgFirstBB); } - else if (info.compInitMem || varDsc->lvMustInit || varTypeIsGC(varDsc) || + 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),