-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Consolidate runtime helpers (X86Base, Interlocked, Buffer) across CoreCLR and NativeAOT #122791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
Co-authored-by: jkotas <[email protected]>
…NativeAOT Co-authored-by: jkotas <[email protected]>
|
@copilot This looks great! Could you please move the QCall that implements Interlocked.MemoryBarrierProcessWide to src\coreclr\runtiem as well? The shared implementation should be called |
Co-authored-by: jkotas <[email protected]>
|
@copilot Let's also move |
src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/X86Base.cs
Outdated
Show resolved
Hide resolved
…emMove to shared MiscNativeHelpers - Added Interlocked_MemoryBarrierProcessWide, Buffer_Clear, and Buffer_MemMove to MiscNativeHelpers.h/cpp - Removed old implementations from VM comutilnative.cpp/h and NativeAOT MiscHelpers.cpp - Updated managed code to use shared QCalls (Interlocked.cs and Buffer.NativeAot.cs) - Removed RhFlushProcessWriteBuffers from NativeAOT RuntimeImports.cs - Defined QCALLTYPE in MiscNativeHelpers.h to ensure compatibility with both VM and NativeAOT - Fixed Buffer_Clear to use direct arithmetic instead of ALIGN_UP/ALIGN_DOWN macros for portability Co-authored-by: jkotas <[email protected]>
Done in 06bba12. Moved Interlocked_MemoryBarrierProcessWide to shared MiscNativeHelpers and removed RhFlushProcessWriteBuffers and VM implementation.
Done in 06bba12. Moved Buffer_Clear and Buffer_MemMove to shared MiscNativeHelpers following the same pattern. |
|
|
||
| // 32 K or OS page | ||
| #define THUNKS_MAP_SIZE (max(0x8000, OS_PAGE_SIZE)) | ||
| #define THUNKS_MAP_SIZE (max((size_t)0x8000, OS_PAGE_SIZE)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add int casts to RhpGetNumThunkBlocksPerMapping and RhpGetNumThunksPerBlock to fix build breaks
return (int)(THUNKS_MAP_SIZE / OS_PAGE_SIZE)
return (int)min(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 092c67b.
| #define THUNKS_MAP_SIZE (max(0x8000, OS_PAGE_SIZE)) | ||
| #define THUNKS_MAP_SIZE (max((size_t)0x8000, OS_PAGE_SIZE)) | ||
|
|
||
| #ifdef TARGET_ARM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot In RhAllocateThunksMapping method, cache THUNKS_MAP_SIZE in a local so that it does not need to be recomputed multiple times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 691b295.
…rBlock - Added (int) cast to return statement in RhpGetNumThunkBlocksPerMapping - Added (int) cast to return statement in RhpGetNumThunksPerBlock - Fixes build breaks caused by size_t to int conversion warnings - Ensures proper return type matching for functions declared as returning int Co-authored-by: jkotas <[email protected]>
- Added size_t thunksMapSize = THUNKS_MAP_SIZE at start of RhAllocateThunksMapping - Replaced all THUNKS_MAP_SIZE usages in the function with thunksMapSize local - Improves performance by avoiding multiple evaluations of the max() macro - Macro involves OS_PAGE_SIZE which could be a non-trivial computation Co-authored-by: jkotas <[email protected]>
|
@copilot Add more casts to fix build breaks |
src/mono/mono/metadata/icall-def.h
Outdated
| NOHANDLES(ICALL(BUFFER_2, "MemmoveInternal", ves_icall_System_Runtime_RuntimeImports_Memmove)) | ||
| NOHANDLES(ICALL(BUFFER_3, "ZeroMemoryInternal", ves_icall_System_Runtime_RuntimeImports_ZeroMemory)) | ||
|
|
||
| ICALL_TYPE(SPANHELPERS, "System.SpanHelpers", SPANHELPERS_0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ICALL_TYPE entries in this file must be sorted alphabetically based on the type name. Move this ICALL_TYPE block down to the right spot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 7a0e6b9.
- Cast OS_PAGE_SIZE to (int) in RhpGetThunkBlockSize return statement - Cast expression to (uint32_t) in thunk address calculation at line 177 - Fixes C4267 warnings about conversion from size_t to int/uint32_t - Ensures proper type matching for return values and assignments Co-authored-by: jkotas <[email protected]>
|
@copilot More build breaks - add casts: |
- Moved ICALL_TYPE(SPANHELPERS) from after System.Buffer to after System.RuntimeTypeHandle - ICALL_TYPE entries must be sorted alphabetically by full type name - System.SpanHelpers now correctly positioned between System.RuntimeTypeHandle and System.String - Maintains proper alphabetical ordering in icall-def.h Co-authored-by: jkotas <[email protected]>
- Cast expression to (uint32_t) in ARM64 thunk generation code - Fixes C4267 warning about conversion from size_t to uint32_t at line 236 - Expression involves OS_PAGE_SIZE calculation for thunk offset - Ensures proper type matching for uint32_t pointer assignment Co-authored-by: jkotas <[email protected]>
Description
Consolidates duplicate implementations from NativeAOT, CoreCLR, and Mono into shared locations, enabling maximum code sharing across all three runtimes. Creates new shared native implementations in
src/coreclr/runtime/MiscNativeHelpers.cpp/hfor QCalls and consolidates managed code in shared System.Private.CoreLib files with minimal conditional compilation.Changes Made
Shared QCall Implementations
X86Base_CpuId - CPUID instruction wrapper
RhCpuIdEx(NativeAOT) andX86BaseCpuId(CoreCLR VM)src/coreclr/runtime/MiscNativeHelpers.cppX86Base.cswith#if MONOconditional compilationX86Base.CoreCLR.cs,X86Base.NativeAot.cs, andX86Base.Mono.cs__cpuidextoCpuIdacross all runtimesInterlocked_MemoryBarrierProcessWide - Process-wide memory barrier
RhFlushProcessWriteBuffers(NativeAOT) and old CoreCLR VM implementationsrc/coreclr/runtime/MiscNativeHelpers.cppBuffer Operations Refactoring
Buffer_ClearandBuffer_MemMoveQCall implementations entirelymemsetandmemmoveinSpanHelpers.ByteMemOps.csSpanHelpers.ByteMemOps.cswith conditional compilation:#if MONO: InternalCall implementations withvoid*pointer types namedmemmoveandmemset#else: LibraryImport QCall implementations for CoreCLR/NativeAOT withvoid*pointer typesBuffer.Mono.csUnsafe.WriteUnalignedwithBlock16for better code generationves_icall_System_SpanHelpers_memmoveandves_icall_System_SpanHelpers_memsetvoid*pointer types matching standard C library conventionsInfrastructure Updates
src/coreclr/runtime/MiscNativeHelpers.h/cppfor shared QCall implementationssrc/coreclr/nativeaot/Runtime/common.hwithQCALLTYPEdefinition for NativeAOTZeroMemorymacro definition in CommonMacros.h to use proper parameter namesINLINEmacro definition that was causing build conflictsCode Sharing Achievement
#if MONO/#elseblocks where runtime-specific behavior is requiredvoid*pointer types for memset/memmove, matching C library conventionsCustomer Impact
None. Internal refactoring only - no functional changes or public API modifications.
Regression
No. This is code consolidation and optimization, not a bug fix.
Testing
Risk
Low. Code consolidation with careful attention to maintaining identical behavior:
Original prompt
This pull request was created from Copilot chat.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.