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

Implement getStaticFieldCurrentClass for NAOT #96982

Closed
wants to merge 8 commits into from

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jan 15, 2024

Closes #96963

@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 Jan 15, 2024
@ghost ghost assigned EgorBo Jan 15, 2024
@ghost
Copy link

ghost commented Jan 15, 2024

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

Issue Details

Closes #96963

Author: EgorBo
Assignees: EgorBo
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo
Copy link
Member Author

EgorBo commented Jan 15, 2024

/azp list

This comment was marked as resolved.

@EgorBo
Copy link
Member Author

EgorBo commented Jan 15, 2024

@MihuBot

@EgorBo
Copy link
Member Author

EgorBo commented Jan 15, 2024

/azp run runtime-coreclr outerloop, runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Jan 15, 2024

@MihuBot

@EgorBo
Copy link
Member Author

EgorBo commented Jan 15, 2024

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Jan 15, 2024

/azp run runtime-nativeaot-outerloop

@EgorBo
Copy link
Member Author

EgorBo commented Jan 15, 2024

@MihuBot

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EgorBo EgorBo marked this pull request as ready for review January 15, 2024 22:12
@EgorBo
Copy link
Member Author

EgorBo commented Jan 16, 2024

@MihuBot

// it can rely on them being invariant too.
if (fieldDesc.HasRva)
{
// Read-only RVA fields need no "is class initialized" check.
Copy link
Member

Choose a reason for hiding this comment

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

How do you know that the RVA field is read-only?

Copy link
Member Author

@EgorBo EgorBo Jan 16, 2024

Choose a reason for hiding this comment

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

How do you know that the RVA field is read-only?

Can it be not so? I thought RVA can be mutable only for C++/CLI. Here we check that the field is final.

Copy link
Member

Choose a reason for hiding this comment

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

We have several of these compiler generated RVA fields. Some of them are read-only, some of them are mutable.

Here we check that the field is final.

Where do we check that?

src/coreclr/jit/gentree.cpp Outdated Show resolved Hide resolved
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@@ -2395,5 +2395,36 @@ private bool notifyMethodInfoUsage(CORINFO_METHOD_STRUCT_* ftn)
{
return true;
}

private CORINFO_CLASS_STRUCT_* getStaticFieldCurrentClass(CORINFO_FIELD_STRUCT_* field, byte* pIsSpeculative)
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused about the expected semantics of this API.

// Return field's exact type if its value is known to be never changed and
// is not null (for reference types). Returns nullptr otherwise.

Like Jan said - we don't check the initonly part - neither for the RVA nor for non-RVA statics. Should this always return null for !initonly?
We also return the type of the field, not the type of the value stored in the field (in the reference type case).

@EgorBo EgorBo marked this pull request as draft May 2, 2024 11:06
@EgorBo EgorBo closed this May 24, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 24, 2024
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.

NAOT: Run-time ISA.IsSupported should be invariant
3 participants