Deleting field sequences from `LCL_FLD` and `VNF_PtrToArrElem` #68986

SingleAccretion · 2022-05-06T20:38:38Z

The first round of deleting field sequences:

From LCL_FLD: replace it with a ClassLayout* pointer (for now, unused). De-pessimize block morphing and block numbering, remove quirks from VN.
From VNF_PtrToArrElem: not replaced by anything because it is not needed anymore.

Also includes clean up of DefinesLocalAddr and a miscellaneous fix for the numbering of return buffer locals.

The diffs are pretty nice, including the throughput ones, and come mainly from the deletion of fgMorphCanUseLclFldForCopy, but also from the array change. We see some regressions with CSEs and hoisting (there is one extreme case of this on x86 where 6 new SIMD CSEs trigger pathological RA behavior and we see it "spill the world", fortunately it is in a PMI-instantiated method). There are also a few cases where locals become tracked and optRemoveRedundantZeroInits no longer removes the explicit zero-initialization.

This also saves ~0.16% in memory consumption when CG-ing CoreLib.

Closes #12142.

ghost · 2022-05-06T20:38:45Z

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

Issue Details

The first round of deleting field sequences:

From LCL_FLD: replace it with a ClassLayout* pointer (unused for now). De-pessimize block morphing and block numbering.
From VNF_PtrToArrElem: not replaced by anything because it is not needed anymore.

Also includes clean up of DefinesLocalAddr and a miscellaneous fix for the numbering of return buffer locals.

We're expecting a good amount of positive diffs, mainly from the deletion of fgMorphCanUseLclFldForCopy, but also from the array change.

Depends on #68979.

Author:	SingleAccretion
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

SingleAccretion · 2022-05-07T20:16:51Z

@dotnet/jit-contrib I would like to request a run of stress and libraries stress for this change.

jakobbotsch · 2022-05-07T21:18:24Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

azure-pipelines · 2022-05-07T21:18:44Z

Azure Pipelines successfully started running 2 pipeline(s).

SingleAccretion · 2022-05-15T11:51:43Z

@jakobbotsch could you please re-trigger the stress pipelines?

jakobbotsch · 2022-05-15T12:01:56Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

azure-pipelines · 2022-05-15T12:02:19Z

Azure Pipelines successfully started running 2 pipeline(s).

SingleAccretion · 2022-05-16T12:50:34Z

That was quite a bit of red CI to interpret...

The runtime pipeline:

Installer Build and Test coreclr Linux_x64 Release: looks like an infrastructure issue.
Libraries Test Run checked coreclr windows x86 Debug: two failures, in both cases the actual tests passed.
Installer Build and Test coreclr windows_x64 Release: Host Test failures in rolling CI #69114.
Windows ARM64 timeout.

Stress:

Linux arm checked: Test failure tracing/runtimeeventsource/nativeruntimeeventsource/nativeruntimeeventsource.sh #68690.
Linux arm64 checked, Linux x64 checked, OSX arm64 checked, windows x64 checked, windows x86 checked: Interop\StringMarshalling\UTF8\UTF8Test failing in outerloop #69349.
Windows ARM64 timeout.

Libraries stress:

Linux ARM Release, Linux ARM64 Release: libraries-jitstress failure in System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo #69154 plus what looks like [Linux][arm] System.Memory.Tests failed with SIGSEGV on rolling build #60825 - a GC hole in System.Memory.Tests. I am however suspicious: there are two asserts, and a segfault on ARM64, and I do not see anything like that in the recent history of the libraries stress pipeline. It seems likely to be related; unfortunately, I am yet to figure out how can one look into an ARM dump without an ARM/64 machine.
Linux x64 Release, windows x64 Release: libraries-jitstress failure in System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo #69154.
windows x86 Release: libraries-jitstress failure in System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo #69154 and VB tests failed without a test summary, which did not reproduce locally.
Windows ARM64 timeout.

jakobbotsch · 2022-05-16T13:06:00Z

I am however suspicious: there are two asserts, and a segfault on ARM64, and I do not see anything like that in the recent history of the libraries stress pipeline. It seems likely to be related; unfortunately, I am yet to figure out how can one look into an ARM dump without an ARM/64 machine.

In theory it should be loadable with x86 windbg, but I have not had much success. I can try looking at the dump on my rpi some time this week.

Run to make sure I don't lose it

jakobbotsch · 2022-05-17T13:34:36Z

Unfortunate I ran into problems while analyzing the dump, I left a comment at
dotnet/diagnostics#2947 (comment).
Let's try a rerun.

jakobbotsch · 2022-05-17T13:34:43Z

/azp run runtime-coreclr libraries-jitstress

SingleAccretion · 2022-05-18T19:47:40Z

@jakobbotsch I think this is ready for another stress run.

jakobbotsch · 2022-05-18T20:11:39Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

azure-pipelines · 2022-05-18T20:11:59Z

Azure Pipelines successfully started running 2 pipeline(s).

SingleAccretion · 2022-05-19T11:29:31Z

The System.Memory.Tests failures are out (as expected).

Linux ARM64/x64 timeouts without logs are in.
Windows ARM64 has one test failure and one heap corruption, so there is another GC-hole-like issue to investigate, TBD how, however (no dumps :( ).

Edit: regular CI hit the same crash inside S.T.J.Tests on Linux ARM.

Edit: two more failures on Linux_musl ARM. This is a bit surprising, the CI was green for this change yesterday, today's force push just removed the GC hole fix (now in main) from the commit list. So it almost seems like something in the upstream commit chain made the failures more apparent.

Edit: and said failures (or at least one of them) do appear to exist in the upstream too, e. g. the stack we see for System.Runtime.Tests AV is identical to the one seen in #20220518.16:

===========================================================================================================
/root/helix/work/workitem/e /root/helix/work/workitem/e
  Discovering: System.Runtime.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Runtime.Tests (found 8065 of 8111 test cases)
  Starting:    System.Runtime.Tests (parallel test collections = on, max threads = 4)
Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at System.Numerics.BigInteger.AssertValid()
   at System.Numerics.BigInteger.CompareTo(System.Numerics.BigInteger)
   at System.Numerics.BigInteger.op_GreaterThan(System.Numerics.BigInteger, System.Numerics.BigInteger)
   at System.Tests.DecimalTests+BigDecimal.ScaleResult(System.Numerics.BigInteger ByRef, Int32 ByRef, Boolean)
   at System.Tests.DecimalTests+BigDecimal.Mul(BigDecimal, Boolean ByRef)
   at System.Tests.DecimalTests+BigIntegerMul.Test()
   at System.RuntimeMethodHandle.InvokeMethod(System.Object, Void**, System.Signature, Boolean)
   at System.Reflection.RuntimeMethodInfo.InvokeNonEmitUnsafe(System.Object, IntPtr*, System.Span`1<System.Object>, System.Reflection.BindingFlags)

Edit: more similar-looking AVs in another stress run.

Edit: on progress status - I have managed to set up things such that I can look into the ARM dumps with x86 WinDbg, next step is to download (1GB worth of) the S.T.J. minidump from the latest stress run, as the core dumps from regular CI do not appear to have enough managed information.

jakobbotsch · 2022-05-21T12:33:14Z

@SingleAccretion I think given your investigations above that you don't need to block this PR on those GC holes. Are there any other known problems with this PR?

SingleAccretion · 2022-05-21T13:58:31Z

@jakobbotsch nothing that I am aware of.

I will investigate the dumps over the weekend, and if nothing comes up, will mark this as ready for review.

Edit, progress status: in all apparency the 1GB S.T.J. dump was too much for the x86 address space to handle (with WinDbg crashing). Perhaps I will be able to get something smaller from Sunday's libraries stress run...

Edit: we have a Linux ARM64 crash, but the dump was too big to upload...

Edit: will presume the GC holes above are #69657.

Move the responsibility of determining "entireness" to its only caller that needed to do that: "DefinesLocal". Add function headers. No diffs.

No diffs.

Also address the TODO-CQ.

Exposed by the more aggressive hoisting.

SingleAccretion · 2022-05-23T16:29:50Z

This is finally ready for review.

Not sure what's up with Libraries Build windows x64 Debug.

@dotnet/jit-contrib, @jakobbotsch

jakobbotsch · 2022-05-27T15:42:24Z

src/coreclr/jit/lclmorph.cpp

-                            {
-
-                                haveCorrectFieldForVN =
-                                    compiler->info.compCompHnd->doesFieldBelongToClass(field->gtFldHnd, clsHnd);


This removes the only uses of this JIT-EE function, correct?

Yep. Will open an issue tracking deleting it completely.

Sounds good. @dotnet/jit-contrib we should keep in mind to delete this one together with the next JIT-EE update.

Also cc @sandreenko who will be happy to hear that :-)

jakobbotsch

Looks great to me, I love to see all the deletions :)

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 6, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label May 6, 2022

SingleAccretion changed the title ~~Physical vn locals upstream~~ Deleting field sequences from LCL_FLD and VNF_PtrToArrElem May 6, 2022

SingleAccretion force-pushed the Physical-VN-Locals-Upstream branch 5 times, most recently from 5a08623 to 9cd3379 Compare May 7, 2022 17:10

SingleAccretion force-pushed the Physical-VN-Locals-Upstream branch 4 times, most recently from dfccd94 to 05ac8d1 Compare May 10, 2022 15:37

This was referenced May 12, 2022

Remove GT_ADDR nodes #11057

Closed

Physical value numbering #68712

Merged

SingleAccretion force-pushed the Physical-VN-Locals-Upstream branch 4 times, most recently from 615799d to 9c81107 Compare May 15, 2022 11:00

jakobbotsch mentioned this pull request May 17, 2022

sos commands eestack & dumpstack crashed with .net 7.0 dotnet/diagnostics#2947

Closed

SingleAccretion mentioned this pull request May 18, 2022

Expand the GC hole fix for explicitly initialized structs #69501

Merged

SingleAccretion force-pushed the Physical-VN-Locals-Upstream branch from 9c81107 to 7a251eb Compare May 18, 2022 17:02

SingleAccretion force-pushed the Physical-VN-Locals-Upstream branch from 7a251eb to a4efcfd Compare May 19, 2022 11:29

SingleAccretion mentioned this pull request May 21, 2022

Use ClassLayout::AreCompatible in fgMorphCanUseLclFldForCopy #69642

Closed

SingleAccretion mentioned this pull request May 22, 2022

ARM32/ARM64 gcstress-extra failures #69657

Closed

SingleAccretion added 6 commits May 23, 2022 16:45

Delete field sequences from GenTreeLclFld

33016f1

Delete field sequences from VNF_PtrToArrElem

c7c1946

Clean up "DefinesLocalAddr"

1e924ac

Move the responsibility of determining "entireness" to its only caller that needed to do that: "DefinesLocal". Add function headers. No diffs.

Also clean up block morphing a little

1562d2a

No diffs.

Fix a potential bug with return buffer numbering

836eac0

Also address the TODO-CQ.

Fix another "VN maintainance in morph" bug

9de70e9

Exposed by the more aggressive hoisting.

SingleAccretion force-pushed the Physical-VN-Locals-Upstream branch from a4efcfd to 9de70e9 Compare May 23, 2022 13:45

SingleAccretion marked this pull request as ready for review May 23, 2022 16:29

jakobbotsch self-assigned this May 23, 2022

jakobbotsch self-requested a review May 23, 2022 16:43

jakobbotsch reviewed May 27, 2022

View reviewed changes

jakobbotsch approved these changes May 27, 2022

View reviewed changes

jakobbotsch merged commit f05fa01 into dotnet:main May 27, 2022

SingleAccretion mentioned this pull request May 27, 2022

Delete doesFieldBelongToClass #69900

Closed

jakobbotsch mentioned this pull request May 29, 2022

Store actual signature types in CallArg and normalize to that type #69969

Merged

SingleAccretion deleted the Physical-VN-Locals-Upstream branch June 7, 2022 16:28

SingleAccretion mentioned this pull request Jun 21, 2022

Delete fgMorphGetStructAddr #71078

Merged

ghost locked as resolved and limited conversation to collaborators Jul 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deleting field sequences from `LCL_FLD` and `VNF_PtrToArrElem` #68986

Deleting field sequences from `LCL_FLD` and `VNF_PtrToArrElem` #68986

SingleAccretion commented May 6, 2022 •

edited

Loading

ghost commented May 6, 2022

SingleAccretion commented May 7, 2022

jakobbotsch commented May 7, 2022

azure-pipelines bot commented May 7, 2022

SingleAccretion commented May 15, 2022

jakobbotsch commented May 15, 2022

azure-pipelines bot commented May 15, 2022

SingleAccretion commented May 16, 2022 •

edited

Loading

jakobbotsch commented May 16, 2022 •

edited

Loading

jakobbotsch commented May 17, 2022

jakobbotsch commented May 17, 2022

SingleAccretion commented May 18, 2022

jakobbotsch commented May 18, 2022

azure-pipelines bot commented May 18, 2022

SingleAccretion commented May 19, 2022 •

edited

Loading

jakobbotsch commented May 21, 2022

SingleAccretion commented May 21, 2022 •

edited

Loading

SingleAccretion commented May 23, 2022

jakobbotsch May 27, 2022

SingleAccretion May 27, 2022

jakobbotsch May 27, 2022

jakobbotsch May 27, 2022

jakobbotsch left a comment

Deleting field sequences from LCL_FLD and VNF_PtrToArrElem #68986

Deleting field sequences from LCL_FLD and VNF_PtrToArrElem #68986

Conversation

SingleAccretion commented May 6, 2022 • edited Loading

ghost commented May 6, 2022

SingleAccretion commented May 7, 2022

jakobbotsch commented May 7, 2022

azure-pipelines bot commented May 7, 2022

SingleAccretion commented May 15, 2022

jakobbotsch commented May 15, 2022

azure-pipelines bot commented May 15, 2022

SingleAccretion commented May 16, 2022 • edited Loading

jakobbotsch commented May 16, 2022 • edited Loading

jakobbotsch commented May 17, 2022

jakobbotsch commented May 17, 2022

SingleAccretion commented May 18, 2022

jakobbotsch commented May 18, 2022

azure-pipelines bot commented May 18, 2022

SingleAccretion commented May 19, 2022 • edited Loading

jakobbotsch commented May 21, 2022

SingleAccretion commented May 21, 2022 • edited Loading

SingleAccretion commented May 23, 2022

jakobbotsch May 27, 2022

Choose a reason for hiding this comment

SingleAccretion May 27, 2022

Choose a reason for hiding this comment

jakobbotsch May 27, 2022

Choose a reason for hiding this comment

jakobbotsch May 27, 2022

Choose a reason for hiding this comment

jakobbotsch left a comment

Choose a reason for hiding this comment

Deleting field sequences from `LCL_FLD` and `VNF_PtrToArrElem` #68986

Deleting field sequences from `LCL_FLD` and `VNF_PtrToArrElem` #68986

SingleAccretion commented May 6, 2022 •

edited

Loading

SingleAccretion commented May 16, 2022 •

edited

Loading

jakobbotsch commented May 16, 2022 •

edited

Loading

SingleAccretion commented May 19, 2022 •

edited

Loading

SingleAccretion commented May 21, 2022 •

edited

Loading