Intrinsic support for transient codegen. #115639
Merged
+426
−456
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The previous transient codegen paths only handled lifetime management when the method had no metadata back IL (that is, RVA == 0). The InstanceCalli logic is marked as an intrinsic and has metadata, but that metadata is copied and then modified falling back to a middle ground that the existing logic didn't handle properly. The tricky scenario is when these new instrinsic types were inlined (lifetimes were being ignored). This change fixes that up but did require a bit of clean-up in the UnsafeJitFunction as it relates to the JIT and interpreter interaction.
|
Tagging subscribers to this area: @mangod9 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds intrinsic support for transient codegen by consolidating method-info retrieval into a new worker function, renaming the helper context, and refactoring the JIT/interpreter code path to handle transient lifetimes and jump-stub overflow more cleanly.
- Rename
MethodInfoHelperContexttoMethodInfoWorkerContextand unify handling ingetMethodInfoWorker. - Refactor
UnsafeJitFunctionWorkerandUnsafeJitFunctionto useMethodInfoWorkerContextand introduceJumpStubOverflowCheck. - Remove legacy debug buffers and simplify constructor signatures by removing default parameters.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/vm/jitinterface.h | Forward-declare MethodInfoWorkerContext, replace getTransientMethodInfo with getMethodInfoWorker, update ctor signatures |
| src/coreclr/vm/jitinterface.cpp | Rename helper class, implement getMethodInfoWorker, remove old transient logic, add JumpStubOverflowCheck, refactor UnsafeJitFunction |
Comments suppressed due to low confidence (4)
src/coreclr/vm/jitinterface.cpp:7497
- The new
getMethodInfoWorkerpath, including transient-method handling and inlining logic, isn't covered by existing tests. Add unit or integration tests to validate behavior for both metadata-backed and transient IL methods.
void CEEInfo::getMethodInfoWorker(
src/coreclr/vm/jitinterface.cpp:13446
- [nitpick] The retry loop in
UnsafeJitFunctionuseswhile(true)withcontinue, which can obscure control flow. Consider ado { ... } while (overflowDetected)pattern or an explicit retry label to improve readability.
while (true)
src/coreclr/vm/jitinterface.cpp:13314
- The static flag
g_fAllowRel32is mutated byDetected()without synchronization, leading to potential race conditions in multi-threaded JIT scenarios. Consider using thread-local storage or a lock to guard modifications.
BOOL JumpStubOverflowCheck::g_fAllowRel32 = TRUE;
src/coreclr/vm/jitinterface.h:520
- Removing the default
allowInlining = trueparameter in these constructors changes their API and may break existing call sites. Verify all callers have been updated or restore the default to preserve backward compatibility.
CEECodeGenInfo(MethodDesc* fd, COR_ILMETHOD_DECODER* header, EECodeGenManager* jm, bool allowInlining)
|
The failure looks related |
jkotas
reviewed
May 16, 2025
|
/azp list |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please fix the typo in the title of the PR before merging ;-) |
AaronRobinsonMSFT
changed the title
jkotas
reviewed
May 21, 2025
jkotas
reviewed
May 21, 2025
jkotas
reviewed
May 21, 2025
jkotas
reviewed
May 21, 2025
Open
3 tasks
|
/ba-g Unrelated CoreCLR Android timeout. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The previous transient codegen paths only handled lifetime management when the method had no metadata back IL (that is, RVA == 0). The InstanceCalli scenario creates intrinsic methods which have metadata, but that metadata is copied and then modified falling into a middle ground that the existing logic didn't handle properly. The tricky scenario is when these new instrinsic types were inlined (lifetimes were being ignored). This change fixes that up but did require a bit of clean-up in the
UnsafeJitFunctionas it relates to the JIT and interpreter interaction.Fixes #115429