-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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_PTRinintops.defand handle it in the interpreter loop (interpexec.cpp). - Refactor
StackInfo(removesize), update all construction sites, and addEmitBoxincompiler.h/compiler.cpp. - Update
EmitCallto handlethisTransformcases 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
clsHndis the unboxed type handle, which may be confusing. Consider renaming it tounboxedClsHndto 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
EmitBoxexplaining thatclsHndrefers to the unboxed type being boxed and thatargByRefcontrols whether to useboxorbox.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
janvorli
left a comment
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
sizefield 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