Add ReadyToRun JIT helper roots metadata - #134092
Conversation
Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/crossgen-contrib |
|
@copilot Please address Jan's feedback. Do not add a check to ensure all jit helpers are handled. HOWEVER, add a comment to jithelpers.h that instructs that future changes of the set of jit helpers should update |
Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
Addressed in 99027aa by removing the extra checking and adding the |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 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.
🟡 Changes recommended
Address mapping synchronization and target-specific helper rooting concerns.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds ReadyToRun rooting for CoreLib JIT helper implementations and documents synchronization with jithelpers.h.
Changes:
- Roots helper methods during ReadyToRun compilation.
- Adds maintenance guidance for the helper mapping.
File summaries
| File | Summary |
|---|---|
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs |
Adds JIT helper method rooting logic. |
src/coreclr/inc/jithelpers.h |
Documents synchronization requirements for the helper mapping. |
Review details
Suppressed comments (1)
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs:1330
- These roots are unconditional, but the corresponding entries in
jithelpers.hare target-dependent:DIV/MOD/UDIV/UMODare only dynamic helpers on 32-bit ARM, and the 64-bit division/modulo helpers are only dynamic on targets other than Windows x86 (jithelpers.h:43-87). As a result, every CoreLib ReadyToRun image now compiles and retains implementations that are not JIT helpers for the current target, increasing code size and making the root set differ from the platform-specific helper set. Gate these roots with the same target conditions, or derive the list from the target-specific helper definitions.
RootJitHelperMethod(graph, "System"u8, "Math"u8, "MultiplyChecked");
RootJitHelperMethod(graph, "System"u8, "Math"u8, "DivInt32");
RootJitHelperMethod(graph, "System"u8, "Math"u8, "DivUInt32");
RootJitHelperMethod(graph, "System"u8, "Math"u8, "DivInt64");
RootJitHelperMethod(graph, "System"u8, "Math"u8, "DivUInt64");
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
|
Looks like #134200
|
I do not see these failures on other PRs. I have disabled auto-merge until this is investigated. |
…/create-jit-helpers-root
Root managed JIT helper methods through the compilation root pipeline so they are tracked in the ReadyToRun method index space before WebAssembly function indices are emitted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Point the jithelpers.h maintenance note at the new ReadyToRun compilation root provider. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The advertised JitHelperRoots section emission, consumption, and validation are not implemented.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Resolved since last review (1)
jkotas
left a comment
There was a problem hiding this comment.
Root managed JIT helper methods through the compilation root pipeline so they are tracked in the ReadyToRun method index space before WebAssembly function indices are emitted.
How do we feel about this ordering dependency?


Summary
Note
This PR description was generated by GitHub Copilot.