Revert "Inline CORINFO_HELP_ARRADDR_ST helper call, remove WriteBarrier FCall"#126530
Revert "Inline CORINFO_HELP_ARRADDR_ST helper call, remove WriteBarrier FCall"#126530
Conversation
…er FCall…" This reverts commit a3b2d40.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR reverts #117583 by restoring the prior write-barrier and stelem.ref helper call patterns, removing the recent JIT-side inlining/intrinsic handling.
Changes:
- Reintroduces a callable write-barrier entrypoint (
JIT_WriteBarrier_Callable) and wiresCastHelpers.WriteBarrieras an FCALL to it. - Removes the
RuntimeHelpers.WriteBarrierintrinsic path and related JIT intrinsic plumbing (named intrinsic, importer expansion, helper->user-call mapping). - Reverts the
CORINFO_HELP_ARRADDR_ST“helper-or-user-equivalent” handling back to helper-only detection.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs | Removes RuntimeHelpers.WriteBarrier shim and reverts minor formatting. |
| src/coreclr/vm/wasm/helpers.cpp | Adds a WASM stub for JIT_WriteBarrier_Callable (currently asserts). |
| src/coreclr/vm/riscv64/asmhelpers.S | Adds JIT_WriteBarrier_Callable wrapper that jumps via JIT_WriteBarrier_Loc. |
| src/coreclr/vm/loongarch64/asmhelpers.S | Adds JIT_WriteBarrier_Callable wrapper that jumps via JIT_WriteBarrier_Loc. |
| src/coreclr/vm/jitinterface.h | Declares JIT_WriteBarrier_Callable and defines WriteBarrier_Helper alias to it. |
| src/coreclr/vm/i386/jithelp.S | Adds JIT_WriteBarrier_Callable thunk for Unix i386. |
| src/coreclr/vm/i386/jithelp.asm | Adds JIT_WriteBarrier_Callable thunk for Windows x86. |
| src/coreclr/vm/ecalllist.h | Registers CastHelpers.WriteBarrier FCALL and adds CastHelpers class mapping. |
| src/coreclr/vm/arm64/asmhelpers.S | Adds JIT_WriteBarrier_Callable thunk (moves args to x14/x15 then branches). |
| src/coreclr/vm/arm64/asmhelpers.asm | Adds Windows ARM64 JIT_WriteBarrier_Callable thunk implementation. |
| src/coreclr/vm/arm/asmhelpers.S | Adds ARM32 JIT_WriteBarrier_Callable thunk branching via JIT_WriteBarrier_Loc. |
| src/coreclr/vm/amd64/jithelpers_fast.S | Adds JIT_WriteBarrier_Callable thunk for Unix AMD64. |
| src/coreclr/vm/amd64/JitHelpers_Fast.asm | Adds JIT_WriteBarrier_Callable thunk for Windows AMD64. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs | Introduces CastHelpers.WriteBarrier internalcall and uses it for stelem.ref stores. |
| src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/RuntimeHelpers.cs | Removes NativeAOT test-corelib RuntimeHelpers.WriteBarrier shim. |
| src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/TypeCast.cs | Switches array ref stores to InternalCalls.RhpAssignRef instead of removed shim. |
| src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/InternalCalls.cs | Adds RhpAssignRef runtime import/internalcall declaration. |
| src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/CompilerServices/RuntimeHelpers.cs | Removes NativeAOT RuntimeHelpers.WriteBarrier shim. |
| src/coreclr/jit/namedintrinsiclist.h | Removes RuntimeHelpers.WriteBarrier named intrinsic. |
| src/coreclr/jit/morph.cpp | Reverts ARRADDR_ST morph condition to helper-only. |
| src/coreclr/jit/importercalls.cpp | Removes intrinsic expansion and helper->user-call conversion plumbing. |
| src/coreclr/jit/importer.cpp | Reverts stelem.ref import to emit helper call directly (no user-call conversion). |
| src/coreclr/jit/gentree.h | Removes IsHelperCallOrUserEquivalent declaration. |
| src/coreclr/jit/gentree.cpp | Removes IsHelperCallOrUserEquivalent implementation and related debug display. |
| src/coreclr/jit/compiler.h | Removes helper-to-managed map and impConvertToUserCallAndMarkForInlining. |
src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs
Show resolved
Hide resolved
🤖 Copilot Code Review — PR #126530Note This review was generated by Copilot. Holistic AssessmentMotivation: This reverts PR #117583 which inlined the Approach: The revert correctly restores the FCall-based Summary: ✅ LGTM. This is a clean revert of a complex JIT optimization by a senior maintainer. The CoreCLR changes are a mechanical revert, the NativeAOT adaptation is correct, all architectures are covered, and there are no public API surface changes. Only minor cosmetic observations noted below. Detailed Findings✅ Correctness — CoreCLR FCall restoration is correctThe ✅ Correctness — JIT infrastructure removal is cleanAll the added JIT infrastructure from #117583 is fully removed:
✅ Correctness — NativeAOT adaptation is appropriateThe NativeAOT code is not a pure revert — it switches from ✅ Platform coverage — All architectures implemented
💡 Minor — Whitespace change in shared RuntimeHelpers.csIn 💡 Minor — Doubled comment in ARM64 assemblyIn This appears to be a concatenation of two comment prefixes. Cosmetic only, no functional impact.
|
mangod9
left a comment
There was a problem hiding this comment.
All dumps show the same stack, so reverting this is reasonable. Will let the analysis run a little longer to determine if it can find the actual root cause within the PR:
Key Finding: NOT ARM64-specific — affects ALL architectures
All 5 newly analyzed crash reports show the exact same stack:
[SIGSEGV] → IsInstanceOfAny_NoCacheLookup
→ CastHelpers.StelemRef_Helper_NoCacheLookup
→ CastHelpers.StelemRef_Helper
→ SZArrayHelper.set_Item<__Canon> ← StelemRef INLINED here
→ IList_Generic_Tests.IList_Generic_ItemSet_LastItemToNonDefaultValue
→ (xunit reflection invoke)
|
/ba-g build analysis stuck |
|
more detailed analysis: @EgorBo, please review the analysis and proposed fix for accuracy and possibly reapply your PR. |
Unfortunately, it doesn't make a lot of sense to me, e.g. it mentions |
…ier FCall" (dotnet#126530) This reverts commit ab2f538.
Ok will check why it came to that conclusion. Is the issue with inlining understood though which causes it to deref the wrong MT? |
Hard to tell honestly 😞. I assume there is a chance it halucinated the LLDB dump or it's 100% real? |
Dumps should be real, since it had access to many and overall dump analysis looks quite genuine. I let it run little longer and used two different models and both are standing by its analysis. Its changed its theory for how isExact gets set in step 2 below. 🤷 |
|
@mangod9 thanks, I'll give it a try once CI back in shape |
…er FCall" (dotnet#126530) Reverts dotnet#117583 Expected to fix dotnet#126516
Reverts #117583
Expected to fix #126516