Remove ref struct pinning from locals

[MichalPetryka]

Removes pinned from ref struct locals as refs to them can't point to the heap legally.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Removes pinned from ref struct locals as refs to them can't point to the heap legally.

Author:	MichalPetryka
Assignees:	-
Labels:	area-CodeGen-coreclr, community-contribution
Milestone:	-

[jkotas]

What is the invalid type info problem with crossgen?

[MichalPetryka]

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.

[jkotas]

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.

[MichalPetryka]

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

[jkotas]

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

[MichalPetryka]

@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?

[jkotas]

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.

[jakobbotsch]

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.

[ryujit-bot]

Diff results for #97997

Assembly diffs

Assembly diffs for osx/arm64 ran on windows/x64

Diffs are based on 1,731,165 contexts (559,643 MinOpts, 1,171,522 FullOpts).

MISSED contexts: base: 227 (0.01%), diff: 5,356 (0.31%)

Overall (-12 bytes)

Collection	Base size (bytes)	Diff size (bytes)
libraries.pmi.osx.arm64.checked.mch	79,814,192	-12

FullOpts (-12 bytes)

Collection	Base size (bytes)	Diff size (bytes)
libraries.pmi.osx.arm64.checked.mch	79,693,064	-12

Details here

---

Throughput diffs

Throughput diffs for windows/arm64 ran on windows/x64

MinOpts (-0.00% to +0.01%)

Collection	PDIFF
libraries.pmi.windows.arm64.checked.mch	+0.01%

Details here

---

[VSadov]

Just curious - what are the savings here? Pinning references that happen to be stack refs seems like mostly a noop - basically just a different bit in GCinfo with no effect on GC. Once GC figures that byref does not point to the heap (which it needs to figure regardless), it will not care about pinning. Actually will stop caring about the reference entirely.

Are there some possible savings on the JIT side?

[MichalPetryka]

Just curious - what are the savings here? Pinning references that happen to be stack refs seems like mostly a noop - basically just a different bit in GCinfo with no effect on GC. Once GC figures that byref does not point to the heap (which it needs to figure regardless), it will not care about pinning. Actually will stop caring about the reference entirely.

Are there some possible savings on the JIT side?

The motivation was to improve the codegen a bit as some people use such pinning to workaround the lack of Unsafe for ref structs.

Example:

Before:

After:

[VSadov]

The motivation was to improve the codegen a bit

I see. That is basically because pinned locals are currently never enregistered, so pinning will force a local to the stack.
I kind of suspected that, just was not sure if it can cause a real grief to people or perhaps it is something else.

[Sergio0694]

Does this also ignores pinning for fields of ref struct types (pointed to by ref)? Eg.:

fixed (long* p = &r2.a)
{
}

(using the same types and names as that snippet in #97997 (comment))

[MichalPetryka]

Does this also ignores pinning for fields of ref struct types (pointed to by ref)? Eg.:

fixed (long* p = &r2.a)
{
}

(using the same types and names as that snippet in #97997 (comment))

No as the local is not a ref to a ref struct in such case.

[Sergio0694]

Not sure I'm following. What I meant is, if you have:

ref struct MyRefStruct
{
    public int X;
}

void Test(ref MyRefStruct value)
{
    fixed (int* p = &value.X)
    {
        DoStuff(p);
    }
}

Shouldn't it also be possible to ignore the pinning on that X field? 🤔

[MichalPetryka]

Not sure I'm following. What I meant is, if you have:

ref struct MyRefStruct
{
    public int X;
}

void Test(ref MyRefStruct value)
{
    fixed (int* p = &value.X)
    {
        DoStuff(p);
    }
}

Shouldn't it also be possible to ignore the pinning on that X field? 🤔

The optimization here is done before the JIT even looks at the IL opcodes so it doesn't know what's assigned to the local, the case you've mentioned would need to be handled later on by looking at the struct layout the FIELD_ADDR for X is coming from and checking if the type there is a ref struct. I think it should be possible but I'm not 100% sure if "unpinning" the local would have any issues then.

[jakobbotsch]

Unfortunately I don't think the impact of the optimization warrants us changing the model of what kind of byref aliasing we consider legal within the JIT. If we could demonstrate some larger impact, then I wouldn't be against reconsidering. Thanks!