-
Notifications
You must be signed in to change notification settings - Fork 5k
Add constrained support for calls #116702
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
Conversation
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
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.
Pull Request Overview
This PR adds support for constrained calls by introducing a new box.ptr
opcode and associated handling, refactors StackInfo
to remove its unused size
field, and implements a shared EmitBox
helper to consolidate boxing logic.
- Introduce
INTOP_BOX_PTR
inintops.def
and handle it in the interpreter loop (interpexec.cpp
). - Refactor
StackInfo
(removesize
), update all construction sites, and addEmitBox
incompiler.h
/compiler.cpp
. - Update
EmitCall
to handlethisTransform
cases including boxing/dereferencing, and remove<CLRTestPriority>
from the test project.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/tests/Loader/classloader/regressions/208900/bug.csproj | Removed outdated <CLRTestPriority> property |
src/coreclr/vm/interpexec.cpp | Added INTOP_BOX_PTR case and updated unboxed/boxed data logic |
src/coreclr/interpreter/intops.def | Defined new OPDEF(INTOP_BOX_PTR, "box.ptr", ...) |
src/coreclr/interpreter/compiler.h | Removed size from StackInfo ; added EmitBox declaration and renamed EmitCall param |
src/coreclr/interpreter/compiler.cpp | Updated PushTypeExplicit , EmitConv , clause init, and EmitCall ; implemented EmitBox helper |
Comments suppressed due to low confidence (2)
src/coreclr/interpreter/compiler.h:499
- [nitpick] The parameter
clsHnd
is the unboxed type handle, which may be confusing. Consider renaming it tounboxedClsHnd
to make its purpose clearer.
void EmitBox(StackInfo* pStackInfo, CORINFO_CLASS_HANDLE clsHnd, bool argByRef);
src/coreclr/interpreter/compiler.cpp:2898
- [nitpick] Add a brief comment above
EmitBox
explaining thatclsHnd
refers to the unboxed type being boxed and thatargByRef
controls whether to usebox
orbox.ptr
.
void InterpCompiler::EmitBox(StackInfo* pStackInfo, CORINFO_CLASS_HANDLE clsHnd, bool argByRef)
@davidwrighton When using gshared types that need to be resolved at runtime, we currently add specific opcodes to do the resolve and store the result in a var. I'm not really a big fan of this approach because it requires indirection, bloats the code with instructions and it might require to add a fair amount of new opcodes, for the gshared version that loads the type from an argument var. I'm thinking that a less invasive approach could be to have the same format for all instructions and when an instruction receives a generic handle as a data item, it either represents the actual type, or if the handle pointer is tagged it represents some other data that is used to resolve the type at runtime as part of the instruction execution. What do you think about this alternative approach ? I could give it a go if it is not on your radar. |
To prevent having uninitialized fields
284db1c
to
f0623fa
Compare
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.
LGTM modulo the test priority change.
Rename constrainedClass to pConstrainedToken for clarity. Extract box code emit into separate method.
It doesn't seem like we will be needing it. Use constructor in more places
f0623fa
to
220d60e
Compare
Plus minor refactor around StackInfo. Remove
size
field since it doesn't look like we will ever use it.Currently there is no support for boxing with generic shared types.
Fixes
Loader/classloader/regressions/208900