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

Remove ref struct pinning from locals #97997

Closed
wants to merge 2 commits into from
Closed

Remove ref struct pinning from locals #97997

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd2a647
Remove ref struct pinning from locals
MichalPetryka Feb 5, 2024
56c1823
Avoid running in R2R
MichalPetryka Feb 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/coreclr/jit/lclvars.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,26 @@ void Compiler::lvaInitTypeRef()

if ((corInfoTypeWithMod & CORINFO_TYPE_MOD_PINNED) != 0)
{
if ((corInfoType == CORINFO_TYPE_CLASS) || (corInfoType == CORINFO_TYPE_BYREF))
CORINFO_CLASS_HANDLE refClsHnd = NO_CLASS_HANDLE;
// don't run in crossgen to avoid invalid type info
Copy link
Member

Choose a reason for hiding this comment

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

What is the invalid type info problem with crossgen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

R2R could be compiled against an assembly version where the type was flagged as byref-like while it was changed to a normal struct later.

Copy link
Member

Choose a reason for hiding this comment

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

Changing byref-like struct to non-byref like struct is not a compatible change. Once the struct is byref-like in public surface, it has to stay byref-like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if so, it's safer to just skip running it there.

Copy link
Member

Choose a reason for hiding this comment

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

We want AOT compilers to have same optimizations as JIT where possible.

if (corInfoType == CORINFO_TYPE_BYREF && !opts.IsReadyToRun())
{
CORINFO_CLASS_HANDLE clsHnd = info.compCompHnd->getArgClass(&info.compMethodInfo->locals, localsSig);

CORINFO_CLASS_HANDLE realClsHnd = NO_CLASS_HANDLE;
if (info.compCompHnd->getChildType(clsHnd, &realClsHnd) == CORINFO_TYPE_VALUECLASS)
{
refClsHnd = realClsHnd;
}
}

// ignore pinned on ref structs
if ((refClsHnd != NO_CLASS_HANDLE) &&
((info.compCompHnd->getClassAttribs(refClsHnd) & CORINFO_FLG_BYREF_LIKE) != 0))
{
JITDUMP("Ignoring pinned on local V%02u for byref-like type %s\n", varNum, eeGetClassName(refClsHnd));
Copy link
Contributor Author

@MichalPetryka MichalPetryka Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jkotas We were having a discussion on Discord whether enforcing ref struct refs not pointing to the heap is an acceptable assumption for the JIT to make, could you give your opinion on the safety of this change?

Copy link
Member

Choose a reason for hiding this comment

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

It is fine to assume that ref struct is pinned and allocated on the stack. It would be a major breaking change (for the R2R format at least) to allocate ref structs on the GC heap.

Copy link
Member

Choose a reason for hiding this comment

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

The concern is about aliasing. Before this change the JIT's aliasing model allows roundtripping any ref A as a ref B. The optimization in this PR is not valid unless we consider that to be UB when A and B do not match in byref likeness.

}
else if ((corInfoType == CORINFO_TYPE_CLASS) || (corInfoType == CORINFO_TYPE_BYREF))
{
JITDUMP("Setting lvPinned for V%02u\n", varNum);
varDsc->lvPinned = 1;
Expand Down
Loading