Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support ByRefLike types as Generic parameters #67783

Merged
merged 27 commits into from
Apr 18, 2022
Merged

Support ByRefLike types as Generic parameters #67783

merged 27 commits into from
Apr 18, 2022

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Apr 8, 2022 •
edited
Loading

Design - #67129

Contributes to #65112

Proposed managed API surface for this work - #68002

@AaronRobinsonMSFT AaronRobinsonMSFT added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-TypeSystem-coreclr labels Apr 8, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 7.0.0 milestone Apr 8, 2022
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

jkotas
jkotas reviewed Apr 9, 2022
View reviewed changes
src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
@AaronRobinsonMSFT AaronRobinsonMSFT mentioned this pull request Apr 11, 2022
Merged
@runfoapp runfoapp bot mentioned this pull request Apr 13, 2022
Closed
@AaronRobinsonMSFT
Revert "Update stub on x86."
877377a 
This reverts commit 92bd7af.
@AaronRobinsonMSFT
Create new JITHelper for the slow Box operation.
8d55448 
Rev Crossgen minor version.
Update NativeAOT boxing helpers.
@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as ready for review April 14, 2022 01:06
@AaronRobinsonMSFT AaronRobinsonMSFT mentioned this pull request Apr 14, 2022
Open
13 tasks
jkotas
jkotas reviewed Apr 14, 2022
View reviewed changes
src/coreclr/jit/utils.cpp Outdated Show resolved Hide resolved
@AaronRobinsonMSFT AaronRobinsonMSFT removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) new-api-needs-documentation labels Apr 14, 2022
@AaronRobinsonMSFT
Review feedback.
15608f0 
Remove new JIT helper.
Special case ByRefLike types in the JIT during boxing.
@AaronRobinsonMSFT
Remove more changes from NativeAOT.
9c849ef
@AaronRobinsonMSFT
Copy link
Member Author

@jakobbotsch and @jkotas Can you please take another look.

Big thanks to @jakobbotsch for the offline help and code examples of what to do in the JIT. Very much appreciated.

@jkotas
Copy link
Member

jkotas commented Apr 15, 2022

Looks better than before

jakobbotsch
jakobbotsch reviewed Apr 15, 2022
View reviewed changes
Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits and one question.

src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/compiler.h Show resolved Hide resolved
src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/importer.cpp Show resolved Hide resolved
@AaronRobinsonMSFT
Copy link
Member Author

@jakobbotsch Any other feedback here? I'd appreciate a sign-off from the JIT side just so there is awareness on the changes in this code path.

jakobbotsch
jakobbotsch approved these changes Apr 18, 2022
View reviewed changes
Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JIT changes LGTM.

There is CORINFO_THIS_TRANSFORM::CORINFO_BOX_THIS. Do we need to do anything special for it or will the VM never return it for byref-like types?

@AaronRobinsonMSFT
Copy link
Member Author

Do we need to do anything special for it or will the VM never return it for byref-like types?

Nothing special. This should just fail naturally in the same way it has always failed since it is not valid for ByRefLike types. This is back to the general solution I originally envisioned vs the narrow approach we are now taking given what we actually need.

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 6be799e into dotnet:main Apr 18, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the generic_byreflike branch April 18, 2022 18:18
directhex pushed a commit to directhex/runtime that referenced this pull request Apr 21, 2022
@AaronRobinsonMSFT @directhex
Support ByRefLike types as Generic parameters (dotnet#67783)
a685994 
* Add flag for ByRefLike generic constraint

* Update ILAsm/ILDasm to support byreflike keyword

* Runtime and Libraries tests

* Cast defined int constant until managed API is approved.
@gbalykov gbalykov mentioned this pull request Apr 27, 2022
Closed
@jkotas jkotas mentioned this pull request May 10, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators May 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MichalStrehovsky MichalStrehovsky MichalStrehovsky left review comments

@jkotas jkotas jkotas approved these changes

@jakobbotsch jakobbotsch jakobbotsch approved these changes

Assignees

@AaronRobinsonMSFT AaronRobinsonMSFT

Labels
area-TypeSystem-coreclr
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@AaronRobinsonMSFT @jkotas @jakobbotsch @MichalStrehovsky