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

Inliner: support inlining method with pinned locals #7774

Closed
AndyAyersMS opened this Issue Oct 24, 2016 · 3 comments

Comments

Projects
None yet
3 participants
@AndyAyersMS
Member

AndyAyersMS commented Oct 24, 2016

Quite a few people have been surprised to see that methods with pinned locals (fixed in C#) can't be inlined. We should support inlining such methods where feasible.

In C# a fixed statement is used to prevent managed objects from being relocated by GC, so that raw pointer math and/or GC-unaware code can be used to access the objects. The fixed statement block is conceptually enclosed in a try/finally block to ensure that no matter how control leaves the scope of the fixed, the pinned local variables are unpinned. In our implementation the JIT treats pinned locals as untracked GC lifetimes. Hence unpinning requires that the pinned locals be explicitly nulled out. But in many cases -- in particular when the call site itself is not in a try/catch -- the C# compiler can prove that the unpinning code is unobservable or unreachable and so the try/finally and possibly the unpinning code can be omitted.

This "fixed optimization" done by the C# compiler is both good news and bad news.

Good news: it gets us past a deep-seated limitation in the JIT about inlining methods with EH. If the C# compiler always used try/finally we'd need to solve a much bigger problem to inline any method with pinning.

Bad news: the JIT must now recreate the unpinning logic after inlining, or risk extending the scope of the pinning. To handle the full set of cases the JIT must be prepared to re-introduce the missing try/finally around the body of the inlined code.

The proposal here is to support inlining of methods with pinned locals in the subset of cases where the C# compiler has omitted the try/finally and the JIT determines that likewise no try/finally need be introduced. The JIT will still need to synthesize and add unpinning code.

@AndyAyersMS AndyAyersMS added this to the Future milestone Oct 24, 2016

@AndyAyersMS AndyAyersMS self-assigned this Oct 24, 2016

AndyAyersMS added a commit to AndyAyersMS/coreclr that referenced this issue Oct 25, 2016

Inliner: support inlining of methods with pinned locals
The inliner now can inline a subset of methods with pinned locals --
namely those where the language compiler (eg CSC) can determine that a
try/finally is not necessary to ensure unpinning, and where the jit
likewise an determines that a try/finally is likewise not needed in
the root method.

Generally speaking this allows inlining in cases where the inline method
is fairly simple and does not any contain exception handling, and the
call site is not within a try region.

When inlining methods that have pinned locals and also return a value,
the jit ensures that the return value is spilled to a temp so that the
unpins can be placed just after the inlined method body and won't alter
the return value.

Closes #7774.

AndyAyersMS added a commit to AndyAyersMS/coreclr that referenced this issue Oct 25, 2016

Inliner: support inlining of methods with pinned locals
The inliner now can inline a subset of methods with pinned locals --
namely those where the language compiler (eg CSC) can determine that a
try/finally is not necessary to ensure unpinning, and where the jit
likewise an determines that a try/finally is likewise not needed in
the root method.

Generally speaking this allows inlining in cases where the inline method
is fairly simple and does not any contain exception handling, and the
call site is not within a try region.

When inlining methods that have pinned locals and also return a value,
the jit ensures that the return value is spilled to a temp so that the
unpins can be placed just after the inlined method body and won't alter
the return value.

Closes #7774.

AndyAyersMS added a commit to AndyAyersMS/coreclr that referenced this issue Oct 27, 2016

Inliner: support inlining of methods with pinned locals
The inliner now can inline a subset of methods with pinned locals --
namely those where the language compiler (eg CSC) can determine that a
try/finally is not necessary to ensure unpinning, and where the jit
likewise an determines that a try/finally is likewise not needed in
the root method.

Generally speaking this allows inlining in cases where the inline method
is fairly simple and does not any contain exception handling, and the
call site is not within a try region.

When inlining methods that have pinned locals and also return a value,
the jit ensures that the return value is spilled to a temp so that the
unpins can be placed just after the inlined method body and won't alter
the return value.

Mark FixedAddressValueType as GCStressIncompatible since the "unfixed"
class static may get re-allocated at the same address. This seems to
happen even without these changes but happens much more frequently with
them.

Closes #7774.

@karelz karelz removed the 2 - In Progress label Nov 9, 2016

AndyAyersMS added a commit to AndyAyersMS/coreclr that referenced this issue Nov 10, 2016

Inliner: support inlining of methods with pinned locals
The inliner now can inline a subset of methods with pinned locals --
namely those where the language compiler (eg CSC) can determine that a
try/finally is not necessary to ensure unpinning, and where the jit
likewise an determines that a try/finally is likewise not needed in
the root method.

Generally speaking this allows inlining in cases where the inline method
is fairly simple and does not any contain exception handling, and the
call site is not within a try region.

When inlining methods that have pinned locals and also return a value,
the jit ensures that the return value is spilled to a temp so that the
unpins can be placed just after the inlined method body and won't alter
the return value.

Mark FixedAddressValueType as GCStressIncompatible since the "unfixed"
class static may get re-allocated at the same address. This seems to
happen even without these changes but happens much more frequently with
them.

Closes #7774.

@AndyAyersMS AndyAyersMS closed this in #7788 Nov 11, 2016

@pentp

This comment has been minimized.

Show comment
Hide comment
@pentp

pentp Dec 28, 2016

Collaborator

Is there a reason that pinned locals must be untracked? Deducing liveness info from unpinning code would remove the try/finally blocks, (synthesized) unpinning code and initialization code.

Collaborator

pentp commented Dec 28, 2016

Is there a reason that pinned locals must be untracked? Deducing liveness info from unpinning code would remove the try/finally blocks, (synthesized) unpinning code and initialization code.

@AndyAyersMS

This comment has been minimized.

Show comment
Hide comment
@AndyAyersMS

AndyAyersMS Dec 29, 2016

Member

The immediate reason that pinned locals are untracked is that the GC info reporting requires it to be this way. See for instance here or here where the flag indicating that a GC reference is pinned is only relevant for untracked locals; that flag bit is repurposed for tracked locals to indicate the reference is to this.

One could imagine alternatives, but it's important to keep the GC info compact.

Member

AndyAyersMS commented Dec 29, 2016

The immediate reason that pinned locals are untracked is that the GC info reporting requires it to be this way. See for instance here or here where the flag indicating that a GC reference is pinned is only relevant for untracked locals; that flag bit is repurposed for tracked locals to indicate the reference is to this.

One could imagine alternatives, but it's important to keep the GC info compact.

@pentp

This comment has been minimized.

Show comment
Hide comment
@pentp

pentp Dec 29, 2016

Collaborator

After looking through code using this_OFFSET_FLAG it looks like this and byref can't be set at the same time. If the flags were instead treated as a 2-bit enum (0, byref, this, pinned) it would be possible to encode pinned & tracked locals? It would also allow encoding this for untracked locals.

Collaborator

pentp commented Dec 29, 2016

After looking through code using this_OFFSET_FLAG it looks like this and byref can't be set at the same time. If the flags were instead treated as a 2-bit enum (0, byref, this, pinned) it would be possible to encode pinned & tracked locals? It would also allow encoding this for untracked locals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment