Skip to content
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

[JIT]: Inline TLS field access for GC type #85619

Merged
merged 26 commits into from
May 13, 2023
Merged

Conversation

kunalspathak
Copy link
Member

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

@dotnet-issue-labeler 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 1, 2023
@ghost ghost assigned kunalspathak May 1, 2023
@ghost
Copy link

ghost commented May 1, 2023

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: -

@runfoapp runfoapp bot mentioned this pull request May 2, 2023
@kunalspathak kunalspathak marked this pull request as ready for review May 3, 2023 18:32
@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib
cc: @VSadov

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

Some questions/comments

src/coreclr/jit/helperexpansion.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/helperexpansion.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/appdomain.hpp Outdated Show resolved Hide resolved
if (pTLM == NULL)
return NULL;

return dac_cast<PTR_BYTE>(pTLM->GetPrecomputedGCStaticsBaseHandle());
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

which method are you referring?

src/coreclr/inc/corinfo.h Outdated Show resolved Hide resolved
@kunalspathak
Copy link
Member Author

gcstress is clean except the failure pointed in #85590.

@jkotas
Copy link
Member

jkotas commented May 12, 2023

The VM side LGTM

@@ -3610,7 +3610,7 @@ void MethodContext::recGetThreadLocalFieldInfo(CORINFO_FIELD_HANDLE field, uint3

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

hhm, wondering this doesn't fail during compilation?

Copy link
Member

Choose a reason for hiding this comment

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

We might not normally build with DEBUG_REC defined.

Comment on lines 3641 to 3644
value.offsetOfMaxThreadStaticBlocks = pInfo->offsetOfMaxThreadStaticBlocks;
value.offsetOfThreadLocalStoragePointer = pInfo->offsetOfThreadLocalStoragePointer;
value.offsetOfThreadStaticBlocks = pInfo->offsetOfThreadStaticBlocks;
value.offsetOfGCDataPointer = pInfo->offsetOfGCDataPointer;
Copy link
Member

Choose a reason for hiding this comment

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

Can you fix the alignment?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

value.offsetOfMaxThreadStaticBlocks = pInfo->offsetOfMaxThreadStaticBlocks;
value.offsetOfThreadLocalStoragePointer = pInfo->offsetOfThreadLocalStoragePointer;
value.offsetOfThreadStaticBlocks = pInfo->offsetOfThreadStaticBlocks;
value.offsetOfMaxThreadStaticBlocks = pInfo->offsetOfMaxThreadStaticBlocks;
Copy link
Member

Choose a reason for hiding this comment

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

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.

", offsetOfMaxThreadStaticBlocks-%u, offsetOfThreadLocalStoragePointer-%u, offsetOfThreadStaticBlocks-%u",
value.tlsIndex.handle, value.offsetOfMaxThreadStaticBlocks, value.offsetOfThreadLocalStoragePointer,
value.offsetOfThreadStaticBlocks);
", offsetOfThreadLocalStoragePointer-%u, offsetOfMaxThreadStaticBlocks-%u"
Copy link
Member

Choose a reason for hiding this comment

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

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

pInfo->offsetOfMaxThreadStaticBlocks = value.offsetOfMaxThreadStaticBlocks;
pInfo->offsetOfThreadLocalStoragePointer = value.offsetOfThreadLocalStoragePointer;
pInfo->offsetOfThreadStaticBlocks = value.offsetOfThreadStaticBlocks;
pInfo->offsetOfMaxThreadStaticBlocks = value.offsetOfMaxThreadStaticBlocks;
Copy link
Member

Choose a reason for hiding this comment

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

This function should also take bool isGCType and use that to determine the key.

Also, please align the =

@ghost 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
@kunalspathak
Copy link
Member Author

@BruceForstall - fixed.

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

A few nits

@kunalspathak
Copy link
Member Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants