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.

Sign up for GitHub

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

Improve getStaticFieldContent for classes without cctors #104054

Closed
wants to merge 1 commit into from
Closed

Improve getStaticFieldContent for classes without cctors #104054

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/coreclr/jit/importercalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2815,6 +2815,10 @@ GenTree* Compiler::impCreateSpanIntrinsic(CORINFO_SIG_INFO* sig)

CORINFO_CLASS_HANDLE fieldOwnerHnd = info.compCompHnd->getFieldClass(fieldToken);

// We're expanding RuntimeHelpers.CreateSpan<int>((RuntimeFieldHandle)0x...), thus, we need to
// statically initialize the class containing that RuntimeFieldHandle
Copy link
Member

Choose a reason for hiding this comment

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

I am not following why we need to do this. The comment should explain why we need to do this.

This comment suggests that the non-expanded version of RuntimeHelpers.CreateSpan performs equivalent operation, but I do not see an equivalent operation in non-expanded version of RuntimeHelpers.CreateSpan. I am not sure whether this is the right fix.

Copy link
Member Author

@EgorBo EgorBo Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Case 1: ReadOnlySpan<byte> RVA: sharplab

here JIT triggers class initialization for <PrivateImplementationDetails> when it imports get_Data() method and sees ldsflda for which it calls getFieldInfo and triggers class init in jit time here

Nothing like that happens for CreateSpan case - there is no ld[s]fld opcode anywhere, only ldtoken field

This comment suggests that the non-expanded version of RuntimeHelpers.CreateSpan performs equivalent operation,

Then I guess it should do the same somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe David's PR indeed broke this behavior, but it seems that if we put some real cctor to <PrivateImplementationDetails> - nobody will call it

Copy link
Member

Choose a reason for hiding this comment

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

Then I guess it should do the same somehow?

RuntimeHelpers.CreateSpan and RuntimeHelpers.InitializeArray APIs never triggered static constructors. They are special APIs. Nothing says that they have to trigger static constructor. I do not see a problem with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then I guess it should do the same somehow?

RuntimeHelpers.CreateSpan and RuntimeHelpers.InitializeArray APIs never triggered static constructors. They are special APIs. Nothing says that they have to trigger static constructor. I do not see a problem with it.

Ok, I will leave this problem to @davidwrighton then since I presume we came to conclusion that MethodTable::IsClassInited should return true for such classes

info.compCompHnd->initClass(fieldToken, info.compMethodHnd, impTokenLookupContextHandle);

CORINFO_CLASS_HANDLE fieldClsHnd;
var_types fieldElementType =
JITtype2varType(info.compCompHnd->getFieldType(fieldToken, &fieldClsHnd, fieldOwnerHnd));
Expand Down
Loading