Do not encode safe points with -1 offset. #110845

VSadov · 2024-12-20T01:02:57Z

Same as #104336,
except for JIT fixes that were included in 9.0 as a part of #105596

When the code position after a call site is a safe point, its GC info is now applicable regardless of how instruction pointer has arrived at this position - by executing a call (and unwinding), returning from a call or jumping around the call,...
This invariant holds in both fully and partially interruptible code.

Therefore leaf/nonleaf/faulted-in-partial/faulted-in-interruptible scenarios all can work the same and we no longer need to resort to various disambiguation schemes.
In particular we do not need to pre-adjust safe point positions in partially interruptible code by -1, into the middle of a previous instruction, and then carefully compensate for that in bunch of places.

An additional benefit is that code offsets in GC info are now all instruction-aligned, thus on RISC architectures fewer bits could be used to encode those.

Fixes: #5677

X86: the change does not affect the legacy X86 GC info

VSadov · 2024-12-20T02:27:25Z

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-12-20T02:27:48Z

Azure Pipelines successfully started running 2 pipeline(s).

VSadov · 2024-12-20T18:26:22Z

[runtime-coreclr gcstress0x3-gcstress0xc (Build coreclr Pri1 Runtime Tests Run osx arm64 checked)] fails with timeouts - the same way as in a noop PR (#110846), so it is not something with this change.

VSadov · 2024-12-20T18:32:10Z

I think this is ready for review. CC: @dotnet/jit-contrib

This is really a rebased change as in #104336 (which was approved, but then uncovered stress/JIT issues).

The issues were eventually fixed separately from the whole PR, but by that time it was too close to 9.0 shipping and the actual change did not make it. So here it is.

VSadov · 2024-12-20T18:38:14Z

NB: The change to GCInfo semantics is compat breaking and this PR cannot be merged as-is. We will the min R2R version to be incremented.

@jkotas - I wonder if this, together with removal of ReturnKind (as a follow up to #110799) is enough to update R2R min version?

Alternatively, we can keep this around ready and wait for something else to force minversion change. Usually some kind of new JIT helper forces that eventually.

jkotas · 2024-12-20T19:20:20Z

@jkotas - I wonder if this, together with removal of ReturnKind (as a follow up to #110799) is enough to update R2R min version?

I am fine with that. It is very likely we will need to do R2R breaking changes during .NET 10 to support cDac and other planned work.

VSadov · 2024-12-22T01:13:40Z

The change now includes not recording Return Kind (follow up to #110799) and bumps up the default GCInfo version as well as min major R2R version.

VSadov · 2024-12-22T01:20:02Z

======== Size impact of this change on arm64:
Using NativeAOT compiled System.Collections.Concurrent.Tests.exe for Win-arm64 as an example

Before the change:
22,576,128 bytes

After the change:
22,396,416 bytes

executable size reduction is roughly 179Kb ( ~0.8% )

The reduction in size is not the primary goal (simplifying the invariants is), but it is nice that the size went down as a sideeffect.

We see a larger diff here, compared to #104336 because of additional effects of not recording Return Kind, which also allows more methods to use slim GC Info format.

VSadov · 2024-12-22T01:20:28Z

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-12-22T01:20:48Z

Azure Pipelines successfully started running 2 pipeline(s).

VSadov · 2024-12-22T01:23:43Z

@mikem8361 @tommcdon - this change has effect on GC Info encoding and when merged may need adjustments in diagnostics repo.

jkotas · 2024-12-22T01:27:29Z

src/coreclr/gcinfo/gcinfoencoder.cpp

@@ -776,12 +774,14 @@ void GcInfoEncoder::SetReversePInvokeFrameSlot(INT32 spOffset)
    m_ReversePInvokeFrameSlot = spOffset;
 }

+#ifndef TARGET_X86
 void GcInfoEncoder::SetReturnKind(ReturnKind returnKind)


This is only used on Linux x86. Is there something fundamental that prevents Linux x86 to be switched to the same plan as the other GC encoder platforms? It would make the change simpler.

I know that there is no testing for Linux x86. I would not be worried about it. Since there is no testing, Linux x86 is likely broken anyway.

Hmm. I actually wanted to make this "ifdef", not "ifndef". How does this even pass tests?

x86 GC info is not supposed to be changed.

Ah. This is still the non-x86 GC encoder. The confusion was because this is a part where non-x86 encoder had an ifdef for TARGET_X86, so I assumed it is a shared path between two encoders.
It is not. The reason for original ifdef was just because X86 would have a different default state for the return kind.

Regardless x86 or not, m_ReturnKind is not going to be used by the non-legacy GC encoder. The non-legacy GC info is expressive enough to not require encoding return registers in an additional/special way.
This does not need to be ifdef'd. It should be removed. There is indeed nothing special about X86 here.

I think my lack of knowledge about GC encoder history shows here.

…r not.

VSadov · 2024-12-22T03:15:24Z

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-12-22T03:15:37Z

Azure Pipelines successfully started running 2 pipeline(s).

jkotas

Nice simplification!

VSadov · 2024-12-22T16:34:06Z

Thanks!!

VSadov added 3 commits December 19, 2024 15:57

Do not record safe points with -1 adjustmnt

89455a5

NORMALIZE_CODE_OFFSET on RISC

79718be

denormalize code offsets in ILCompiler.Reflection.ReadyToRun.Amd64

a5b967b

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 20, 2024

dotnet-policy-service bot assigned VSadov Dec 20, 2024

VSadov added area-VM-coreclr area-NativeAOT-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 20, 2024

VSadov marked this pull request as ready for review December 20, 2024 18:26

VSadov requested a review from MichalStrehovsky as a code owner December 20, 2024 18:26

VSadov force-pushed the noIPadjNew branch from 33146e4 to 840ec5f Compare December 21, 2024 06:24

build-analysis bot mentioned this pull request Dec 21, 2024

The hosted runner encountered an error while running your job. (Error Type: Disconnect). dotnet/dnceng#1919

Open

3 tasks

bump the min R2R version and GCInfo version

e27805f

VSadov force-pushed the noIPadjNew branch from 9e5ebac to e27805f Compare December 21, 2024 21:28

Do not record Return Kind

856ebd2

jkotas reviewed Dec 22, 2024

View reviewed changes

m_ReturnKind is not needed in non-legacy GC encoder, regardless X86 o…

f212ed2

…r not.

jkotas approved these changes Dec 22, 2024

View reviewed changes

github-actions bot mentioned this pull request Dec 22, 2024

110845 MichalStrehovsky/rt-sz#93

Closed

VSadov merged commit 0cba4a1 into dotnet:main Dec 22, 2024
138 of 140 checks passed

VSadov deleted the noIPadjNew branch December 22, 2024 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not encode safe points with -1 offset. #110845

Do not encode safe points with -1 offset. #110845

VSadov commented Dec 20, 2024 •

edited

Loading

VSadov commented Dec 20, 2024

azure-pipelines bot commented Dec 20, 2024

VSadov commented Dec 20, 2024

VSadov commented Dec 20, 2024 •

edited

Loading

VSadov commented Dec 20, 2024 •

edited

Loading

jkotas commented Dec 20, 2024 •

edited

Loading

VSadov commented Dec 22, 2024

VSadov commented Dec 22, 2024 •

edited

Loading

VSadov commented Dec 22, 2024

azure-pipelines bot commented Dec 22, 2024

VSadov commented Dec 22, 2024

jkotas Dec 22, 2024

VSadov Dec 22, 2024

VSadov Dec 22, 2024 •

edited

Loading

VSadov Dec 22, 2024

VSadov commented Dec 22, 2024

azure-pipelines bot commented Dec 22, 2024

jkotas left a comment

VSadov commented Dec 22, 2024

Do not encode safe points with -1 offset. #110845

Do not encode safe points with -1 offset. #110845

Conversation

VSadov commented Dec 20, 2024 • edited Loading

VSadov commented Dec 20, 2024

azure-pipelines bot commented Dec 20, 2024

VSadov commented Dec 20, 2024

VSadov commented Dec 20, 2024 • edited Loading

VSadov commented Dec 20, 2024 • edited Loading

jkotas commented Dec 20, 2024 • edited Loading

VSadov commented Dec 22, 2024

VSadov commented Dec 22, 2024 • edited Loading

VSadov commented Dec 22, 2024

azure-pipelines bot commented Dec 22, 2024

VSadov commented Dec 22, 2024

jkotas Dec 22, 2024

Choose a reason for hiding this comment

VSadov Dec 22, 2024

Choose a reason for hiding this comment

VSadov Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

VSadov Dec 22, 2024

Choose a reason for hiding this comment

VSadov commented Dec 22, 2024

azure-pipelines bot commented Dec 22, 2024

jkotas left a comment

Choose a reason for hiding this comment

VSadov commented Dec 22, 2024

VSadov commented Dec 20, 2024 •

edited

Loading

VSadov commented Dec 20, 2024 •

edited

Loading

VSadov commented Dec 20, 2024 •

edited

Loading

jkotas commented Dec 20, 2024 •

edited

Loading

VSadov commented Dec 22, 2024 •

edited

Loading

VSadov Dec 22, 2024 •

edited

Loading