Delete JIT_GetDynamic{GC,NonGC}StaticBase_SingleAppDomain asm helpers - #132670
Conversation
All platforms now use the managed StaticsHelpers.GetDynamic{GC,NonGC}StaticBase,
as x86/arm32 already did.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7624d8d2-999a-494e-9f64-f597c3faf97d
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @agocke |
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR removes the platform-specific JIT_GetDynamic{GC,NonGC}StaticBase_SingleAppDomain assembly helper implementations (and associated constants/wiring) so the relevant CORINFO_HELP_GET(DYNAMIC|PINNED)_*STATIC_BASE helpers consistently resolve through the existing managed StaticsHelpers implementations via the binder-based dynamic helper mechanism.
Changes:
- Remove the SingleAppDomain asm helper implementations for dynamic GC/NonGC static base access on amd64/arm64/loongarch64/riscv64 and drop the corresponding
cgencpu.haliasing. - Remove now-dead
DynamicStaticsInfoasm offset constants and deadg_pGet{GC,NonGC}StaticBaseglobals/initialization. - Switch the affected
DYNAMICJITHELPER(...)entries toNULLfunction pointers so they bind to the managedMETHOD__STATICSHELPERS__*targets uniformly.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/riscv64/cgencpu.h | Removes SingleAppDomain aliasing for dynamic static base helpers. |
| src/coreclr/vm/riscv64/asmhelpers.S | Deletes RISC-V asm implementations of dynamic static base helpers. |
| src/coreclr/vm/riscv64/asmconstants.h | Drops DynamicStaticsInfo offset constants used only by deleted asm helpers. |
| src/coreclr/vm/loongarch64/cgencpu.h | Removes SingleAppDomain aliasing for dynamic static base helpers. |
| src/coreclr/vm/loongarch64/asmhelpers.S | Deletes LoongArch64 asm implementations of dynamic static base helpers. |
| src/coreclr/vm/loongarch64/asmconstants.h | Drops DynamicStaticsInfo offset constants used only by deleted asm helpers. |
| src/coreclr/vm/jitinterface.h | Removes always-NULL shims/declarations for the deleted helper names. |
| src/coreclr/vm/CMakeLists.txt | Removes amd64 single-appdomain asm source files from build inputs. |
| src/coreclr/vm/arm64/cgencpu.h | Removes SingleAppDomain aliasing for dynamic static base helpers. |
| src/coreclr/vm/arm64/asmhelpers.S | Deletes ARM64 Unix asm implementations of dynamic static base helpers. |
| src/coreclr/vm/arm64/asmhelpers.asm | Deletes ARM64 Windows asm implementations and related imports. |
| src/coreclr/vm/arm64/asmconstants.h | Drops DynamicStaticsInfo offset constants used only by deleted asm helpers. |
| src/coreclr/vm/appdomain.cpp | Removes dead g_pGet{GC,NonGC}StaticBase globals and initialization. |
| src/coreclr/vm/amd64/jithelpers_singleappdomain.S | Deletes amd64 Unix single-appdomain helper file. |
| src/coreclr/vm/amd64/JitHelpers_SingleAppDomain.asm | Deletes amd64 Windows single-appdomain helper file. |
| src/coreclr/vm/amd64/cgencpu.h | Removes SingleAppDomain aliasing for dynamic static base helpers. |
| src/coreclr/vm/amd64/asmconstants.h | Drops DynamicStaticsInfo offset constants used only by deleted asm helpers. |
| src/coreclr/inc/jithelpers.h | Switches affected static/dynamic/pinned helper entries to binder-based managed resolution (NULL pfn). |
davidwrighton
left a comment
davidwrighton
left a comment
There was a problem hiding this comment.
As long as you've taken cursory look at perf. I approve.
I've done many ad-hoc benchmarks with ALC (unloadable) to trigger JIT_GetDynamic*, wasn't able to detect any regressions nor improvements. The only observable change is that sometimes the call to the managed helper is indirect, while previously it was always direct. But that happens to all managed helpers - they're emitted as direct call only when they reach Tier1 by the time whoever needs them is being compiled (a special VM optimization for all managed helpers). |
|
/ba-g "publish build assets" failed |
Deletes the hand-written
JIT_GetDynamic{GC,NonGC}StaticBase_SingleAppDomainhelpers on amd64/arm64/loongarch64/riscv64, soCORINFO_HELP_GETDYNAMIC_*STATIC_BASEandCORINFO_HELP_GETPINNED_*STATIC_BASEresolve to the managedStaticsHelpers.GetDynamic{GC,NonGC}StaticBaseon every platform.Why:
jitinterface.hfell back toNULL-> binder there). This is not a new correctness model, just one fewer platform-specific override.DynamicStaticsInfo*back to aMethodTable*and tail-jumped to the non-dynamic managed helper, which re-read the statics pointer and re-tested bit 0. The managed dynamic helper tail-jumps straight toGetNonGCStaticBaseSlow.ret) vs ~30 for the asm.bnez) instead of bit 0, so an initialized base took the slow managed tail call every time. Deleting removes the bug instead of fixing it in four places.jmp JIT_GetDynamicNonGCStaticBase_Portable->jmp [g_pGetNonGCStaticBase]) and wrote the managed dynamic helpers in the same change; they have been unreachable on these four arches ever since. I found no perf concerns recorded anywhere in that PR - there are no review comments on anyasm/cgencpu.h/jitinterface.hfile in it, and the non-Dynamic pair was taken fully managed there.nativeaot/System.Private.CoreLib/.../ClassConstructorRunner.cs).Also drops what became dead:
g_pGetGCStaticBase/g_pGetNonGCStaticBase, theOFFSETOF__DynamicStaticsInfo__*asmconstants on all four arches, and the always-NULLJIT_Get*StaticBaseshims injitinterface.h(the*_NOCTORshims stay - they still resolve to real_Portableimplementations).