TL;DR: JIT has an optimization that tries to simplify/remove write barriers, but that optimization happens too late to rely on VN/SSA/Assertions, so CSE can easily ruin it by moving addresses or values to locals and forcing that opt to give up and use the checked write barrier. This PR assists that optimization from assertion prop (#97901 did it for nongc objects as values for such indirs), Example:

publicstructSlot{publicobjectItem;publicintSequenceNumber;}staticvoidTest(Slot[]arr,objecto){arr[0].Item=o;arr[0].SequenceNumber=1;}

Codegen forTest:

; Method LdtokenRepro:Test(Slot[],System.Object) (FullOpts)G_M35662_IG01:  ;; offset=0x0000       push     rbx       sub      rsp, 32G_M35662_IG02:  ;; offset=0x0005       cmp      dword ptr [rcx+0x08], 0       jbe      SHORT G_M35662_IG04       lea      rbx, bword ptr [rcx+0x10]       mov      rcx, rbx-      call     CORINFO_HELP_CHECKED_ASSIGN_REF+      call     CORINFO_HELP_ASSIGN_REF       mov      dword ptr [rbx+0x08], 1G_M35662_IG03:  ;; offset=0x001E       add      rsp, 32       pop      rbx       ret      G_M35662_IG04:  ;; offset=0x0024       call     CORINFO_HELP_RNGCHKFAIL       int3     ; Total bytes of code: 42

Mark indirs with GTF_IND_TGT_NOT_HEAP and GTF_IND_TGT_HEAP in assertprop

5f91d49

ghost added the area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label

Feb 5, 2024

ghost assignedEgorBo

Feb 5, 2024

Copy link

ghost commentedFeb 5, 2024

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

Issue Details

Closes#97534

TL;DR: JIT has an optimization that tries to simplify/remove write barriers, but that optimization happens to late to rely on VN/SSA/Assertions, so CSE can easily ruin it by moving addresses or values to locals and forcing that opt to give up and use the checked write barrier. This PR assists that optimization from assertion prop, Example:

publicstructSlot{publicobjectItem;publicintSequenceNumber;}staticvoidTest(Slot[]arr,objecto){arr[0].Item=o;arr[0].SequenceNumber=1;}

Codegen forTest:

; Method LdtokenRepro:Test(Slot[],System.Object) (FullOpts)G_M35662_IG01:  ;; offset=0x0000       push     rbx       sub      rsp, 32G_M35662_IG02:  ;; offset=0x0005       cmp      dword ptr [rcx+0x08], 0       jbe      SHORT G_M35662_IG04       lea      rbx, bword ptr [rcx+0x10]       mov      rcx, rbx-      call     CORINFO_HELP_CHECKED_ASSIGN_REF+      call     CORINFO_HELP_ASSIGN_REF       mov      dword ptr [rbx+0x08], 1G_M35662_IG03:  ;; offset=0x001E       add      rsp, 32       pop      rbx       ret      G_M35662_IG04:  ;; offset=0x0024       call     CORINFO_HELP_RNGCHKFAIL       int3     ; Total bytes of code: 42

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

EgorBo added3 commits

February 5, 2024 01:18

clean up

48cdb01

fix compilation

77731bd

Clean up

a1666d7

This was referencedFeb 5, 2024

Tests crashing in CI with no dump: exit code 137 means SIGKILL Killed#97049

Closed

System.Net.Security.Tests.SslStreamCertificateContextOcspLinuxTests.RefreshOcspResponse_BeforeExpiration test failure#97779

Closed

EgorBo added2 commits

February 5, 2024 12:50

Fix TP regression

e79b58b

Merge branch 'main' of github.com:dotnet/runtime into assertprop-wb-f…

2a91eb2

…ix-3

EgorBo marked this pull request as ready for review

February 5, 2024 12:51

Copy link

MemberAuthor

EgorBo commentedFeb 5, 2024

@AndyAyersMS @jakobbotsch cc@SingleAccretion @dotnet/jit-contrib PTAL.Diffs. Diffs aren't too big becauseCORINFO_HELP_CHECKED_ASSIGN_REF ->CORINFO_HELP_ASSIGN_REF change has 0 size diff, but should improve perf since the latter doesn't need to check whether pointer targets heap or not (two comparisons). Most size diffs come fromPtrToLoc ->NoWritebarrier. A small TP regression due to GetVNFunc in a loop (the regression was much bigger before I minimized it), but, hopefully, faster write barriers are worth it.

Closes#97534

Copy link

MemberAuthor

EgorBo commentedFeb 5, 2024

@MihuBot

MihuBot mentioned this pull request

Feb 5, 2024

[X64] [EgorBo] JIT: Use faster write barriers when we know the target address is on the heapMihuBot/runtime-utils#235

Open

This was referencedFeb 5, 2024

Checkout failure: "Git fetch failed with exit code 128"dotnet/arcade#9009

Open

Error while retrieving client Settings for PipelineArtifact. HttpRequestException: nodename nor servname provided, or not known (vsblobprodcus3.vsblob.visualstudio.com:443)#96798

Open

"We stopped hearing from agent Azure Pipelines 32. Verify the agent machine is running and has a healthy network connection"dotnet/dnceng#1886

Open

System.Net.Security.Tests.SslStreamCertificateContextOcspLinuxTests.FetchOcspResponse_FirstInvalidThenValid test failure#97836

Closed

Copy link

MemberAuthor

EgorBo commentedFeb 7, 2024

ping@AndyAyersMS - I presume it should help you with CSE work sinceCORINFO_HELP_ASSIGN_REF andCORINFO_HELP_CHECKED_ASSIGN_REF have the same asm and PerfScore, so CSE changes migth silently regress them.

I analyzed jit-diff on top of BCL and this PR removed ~2400 checked write barriers.

EgorBo commented

Feb 7, 2024

View reviewed changes

src/coreclr/jit/assertionprop.cpp

		if (arg1Type != GCInfo::WriteBarrierForm::WBF_BarrierUnknown)
		{
		return arg1Type;
		}

Copy link

MemberAuthor

EgorBoFeb 7, 2024

Choose a reason for hiding this comment

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

NOTE: this has an assumption that "addressOfLocal + addressOfHeap" is UB

Copy link

MemberAuthor

EgorBoFeb 8, 2024

Choose a reason for hiding this comment

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

Made it a bit more conservative (it didn't affect diffs) by requiring the 2nd argument to be a non-handle constant. So, if we see address being GT_ADD(op1, op2) and one of the arguments is either address-of-local or address-within-heap, the other argument must be a non-handle constant.

kunalspathak approved these changes

Feb 7, 2024

View reviewed changes

Copy link

Contributor

kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

src/coreclr/jit/assertionprop.cpp OutdatedShow resolvedHide resolved

src/coreclr/jit/assertionprop.cppShow resolvedHide resolved

AndyAyersMS approved these changes

Feb 7, 2024

View reviewed changes

Copy link

Member

AndyAyersMS left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing this.

We could / should have perf score reflect the cost of helper calls (likewise with gtCostEx).

EgorBo added3 commits

February 8, 2024 00:56

Merge branch 'main' of github.com:dotnet/runtime into assertprop-wb-f…

9804df4

…ix-3

Address feedback

309f486

Merge branch 'main' of github.com:dotnet/runtime into assertprop-wb-f…

d107c06

…ix-3

Copy link

MemberAuthor

EgorBo commentedFeb 8, 2024

We could / should have perf score reflect the cost of helper calls (likewise with gtCostEx).

I agree, I assume currently all unknown calls have the same cost (assuming same arguments)? Do we want to make all helpers cheaper than unknown calls or more expensive? I presume this could result in some diffs