[JIT]: Inline TLS field access for GC type by kunalspathak · Pull Request #85619 · dotnet/runtime

kunalspathak · 2023-05-01T23:24:30Z

Along the lines of #82973 but for GC type fields.

ghost · 2023-05-01T23:24:39Z

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

Issue Details

Along the lines of #82973 but for GC type fields.

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

kunalspathak · 2023-05-03T18:32:17Z

@dotnet/jit-contrib
cc: @VSadov

BruceForstall

Some questions/comments

BruceForstall · 2023-05-06T00:40:07Z

+ if (pTLM == NULL)
+ return NULL;
+
+ return dac_cast<PTR_BYTE>(pTLM->GetPrecomputedGCStaticsBaseHandle());


This is almost a copy of the function above, but that one doesn't use dac_cast<PTR_BYTE>; is that an omission?

which method are you referring?

kunalspathak · 2023-05-11T04:47:34Z

gcstress is clean except the failure pointed in #85590.

jkotas · 2023-05-12T16:50:41Z

The VM side LGTM

BruceForstall · 2023-05-12T18:31:20Z

 key = CastHandle(field);
 GetThreadLocalFieldInfo->Add(key, result);
- DEBUG_REC(dmpGetThreadLocalFieldInfo(key, result));
+ DEBUG_REC(dmpGetGCThreadLocalFieldInfo(key, result));


This is a bug; the function doesn't exist?

hhm, wondering this doesn't fail during compilation?

We might not normally build with DEBUG_REC defined.

BruceForstall · 2023-05-12T18:31:31Z

+ value.offsetOfMaxThreadStaticBlocks = pInfo->offsetOfMaxThreadStaticBlocks;
+ value.offsetOfThreadLocalStoragePointer = pInfo->offsetOfThreadLocalStoragePointer;
+ value.offsetOfThreadStaticBlocks = pInfo->offsetOfThreadStaticBlocks;
+ value.offsetOfGCDataPointer = pInfo->offsetOfGCDataPointer;


Can you fix the alignment?

seems these projects are excluded from auto-fixing using formatter?

Sergey set up using the formatter, but it's not part of the normal JIT runs or validation in CI.

BruceForstall · 2023-05-12T18:34:33Z

- value.offsetOfMaxThreadStaticBlocks = pInfo->offsetOfMaxThreadStaticBlocks;
- value.offsetOfThreadLocalStoragePointer = pInfo->offsetOfThreadLocalStoragePointer;
- value.offsetOfThreadStaticBlocks = pInfo->offsetOfThreadStaticBlocks;
+ value.offsetOfMaxThreadStaticBlocks = pInfo->offsetOfMaxThreadStaticBlocks;


This function signature needs to match getThreadLocalFieldInfo: it needs to take a bool isGCType.
Then, instead of key 0 the key should be 0 for isGCType == false and 1 for isGCType == true.

BruceForstall · 2023-05-12T18:36:10Z

- ", offsetOfMaxThreadStaticBlocks-%u, offsetOfThreadLocalStoragePointer-%u, offsetOfThreadStaticBlocks-%u",
- value.tlsIndex.handle, value.offsetOfMaxThreadStaticBlocks, value.offsetOfThreadLocalStoragePointer,
- value.offsetOfThreadStaticBlocks);
+ ", offsetOfThreadLocalStoragePointer-%u, offsetOfMaxThreadStaticBlocks-%u"


The key won't always be zero if it's based on isGCType

BruceForstall · 2023-05-12T18:37:01Z

- pInfo->offsetOfMaxThreadStaticBlocks = value.offsetOfMaxThreadStaticBlocks;
- pInfo->offsetOfThreadLocalStoragePointer = value.offsetOfThreadLocalStoragePointer;
- pInfo->offsetOfThreadStaticBlocks = value.offsetOfThreadStaticBlocks;
+ pInfo->offsetOfMaxThreadStaticBlocks = value.offsetOfMaxThreadStaticBlocks;


This function should also take bool isGCType and use that to determine the key.
Also, please align the =

kunalspathak · 2023-05-12T19:36:25Z

@BruceForstall - fixed.

BruceForstall

A few nits

kunalspathak · 2023-05-12T23:37:47Z

#86176 fixed the test build failure, so will make sure that everything is clean before merging.

kunalspathak added 5 commits April 26, 2023 16:40

Add CORINFO_HELP_GETSHARED_GCTHREADSTATIC_BASE_NOCTOR_OPTIMIZED
01af65b

Merge remote-tracking branch 'kp/tls_gc'
9142bcf

Working prototype
abbfe77

Rename ThreadStaticBlock to NonGCThreadStaticBlock
743420e

Add GCThreadStaticBlock
be1914e

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 1, 2023

ghost assigned kunalspathak May 1, 2023

fix linux build
966e7a0

jkotas reviewed May 2, 2023
View reviewed changes

Comment threadsrc/coreclr/vm/appdomain.hpp

runfoappBot mentioned this pull request May 2, 2023
Infra improvements for Helix #68176
Closed

Restrict the optimization to non-generics
26626ad

This was referenced May 3, 2023
Tracking issue for CI build timeouts #76454
Closed
Failures in System.Net.Mail.Tests.SmtpClientTest tests #85637
Closed
System.IO.Tests.RandomAccess_NoBuffering.ReadUsingSingleBuffer timing out #85659
Closed

kunalspathak added 2 commits May 3, 2023 08:13

Update the guid
772d9e7

Add canOptimizeHelper check
4378b96

kunalspathak commented May 3, 2023
View reviewed changes

Comment threadsrc/coreclr/vm/jitinterface.cpp Outdated

Merge remote-tracking branch 'origin/main' into tls_gc
5907d0c

kunalspathak marked this pull request as ready for review May 3, 2023 18:32

kunalspathak requested a review from MichalStrehovsky as a code owner May 3, 2023 18:32

BruceForstall reviewed May 6, 2023
View reviewed changes

kunalspathak added 3 commits May 9, 2023 11:16

Rename and related methods from typeIDMap to NonGCTypeID
269719f

Introduce GCTypeIDMap and getGCThreadLocalFieldInfo()
a9073ab

Use GCThreadLocalFieldInfo()
cea1888

build-analysisBot mentioned this pull request May 9, 2023
Could not load file or assembly 'Microsoft.CodeAnalysis.NetAnalyzers #84995
Closed

kunalspathak added 3 commits May 10, 2023 12:45

fix the bug about gc tracking
7ba1bda

Merge remote-tracking branch 'origin/main' into tls_gc
1290ac6

resolve merge conflicts
bdf0aa9

jkotas reviewed May 11, 2023
View reviewed changes

Comment threadsrc/coreclr/inc/corinfo.h Outdated

kunalspathak added 2 commits May 10, 2023 22:52

Merge branch 'main' into tls_gc
c14f5a2

Unify some of the non-gc/gc methods
bb43e00

jkotas reviewed May 12, 2023
View reviewed changes

Comment threadsrc/coreclr/vm/appdomain.cpp Outdated

kunalspathak added 2 commits May 12, 2023 09:38

fix missing methods
5b8a022

update the comments
5e81673

jkotas reviewed May 12, 2023
View reviewed changes

Comment threadsrc/coreclr/jit/helperexpansion.cpp Outdated

Track gc for derefernce of object handle
8383cf6

BruceForstall suggested changes May 12, 2023
View reviewed changes

ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels May 12, 2023

fix the superpmi methods
1f4c8ef

BruceForstall approved these changes May 12, 2023
View reviewed changes

Comment threadsrc/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp Outdated
Comment threadsrc/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp
Comment threadsrc/coreclr/tools/superpmi/superpmi-shared/methodcontext.h Outdated

review feedback
ef9532e

build-analysisBot mentioned this pull request May 12, 2023
System.Net.Quic.Tests.QuicStreamTests.WriteCanceled_NextWriteThrows test failure #76831
Closed

Merge remote-tracking branch 'origin/main' into tls_gc
69766df

kunalspathak merged commit d08c807 into dotnet:mainMay 13, 2023

kunalspathak deleted the tls_gc branch May 13, 2023 02:36

kunalspathak mentioned this pull request Jun 2, 2023
Inline TLS field access for linux/osx x64/arm64 #87082
Merged

JulieLeeMSFT mentioned this pull request Jun 9, 2023
What's new in .NET 8 Preview 5 [WIP] dotnet/core#8436
Closed
3 tasks

ghost locked as resolved and limited conversation to collaborators Jun 12, 2023

Uh oh!

Conversation

kunalspathak commented May 1, 2023

Uh oh!

ghost commented May 1, 2023

Uh oh!

Uh oh!

Uh oh!

kunalspathak commented May 3, 2023

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kunalspathak commented May 11, 2023

Uh oh!

Uh oh!

Uh oh!

jkotas commented May 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunalspathak commented May 12, 2023

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kunalspathak commented May 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants