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

Improve getStaticFieldContent for classes without cctors #104054

Closed
wants to merge 1 commit into from

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jun 26, 2024

When a class has no static constructor (cctor), its pEnclosingMT->IsClassInited() returns false. It shouldn't stop us from folding RVA fields. Example:

public class Program
{
    static void Main()
    {
        Test();
    }

    static ReadOnlySpan<int> RvaData => [1, 2, 3, 4];

    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Test() => RvaData[2];
}

Codegen of Test() in Main (Tier1 or FullOpts):

; Method Program:Test():int (FullOpts)
       mov      rax, 0x2AEC82D3450
       mov      eax, dword ptr [rax]
       ret

New codegen of Test():

; Method Program:Test():int (FullOpts)
       mov      eax, 3
       ret

PS: I have an impression that pEnclosingMT->IsClassInited() used to return true in such cases 🤔 Or maybe entrypoint holders always used to have cctors? cc @jkotas
Perhaps it's some change from DavidWr's statics PR?

The same snippet works well on NAOT.

@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 Jun 26, 2024
Copy link
Contributor

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

@jkotas jkotas requested a review from davidwrighton June 26, 2024 18:47
@jkotas
Copy link
Member

jkotas commented Jun 26, 2024

Perhaps it's some change from DavidWr's statics PR?

Yes, it looks like a regression from @davidwrighton's change. IsClassInited used to return true for types without static constructor before that change.

Are there other callers of IsClassInited that will hit the same problem?

@EgorBo
Copy link
Member Author

EgorBo commented Jun 27, 2024

Perhaps it's some change from DavidWr's statics PR?

Yes, it looks like a regression from @davidwrighton's change. IsClassInited used to return true for types without static constructor before that change.

Are there other callers of IsClassInited that will hit the same problem?

Hard to say, it seems like the problematic case is <PrivateImplementationDetails> class (RVA fields holder, I guess)

@EgorBo EgorBo force-pushed the fix-getStaticFieldContent-rva branch from b2dff30 to 3673427 Compare June 30, 2024 22:20
@EgorBo
Copy link
Member Author

EgorBo commented Jun 30, 2024

@jkotas actually, the problem is on the JIT side since the same snippet works when I use byte for RVA.

It seems that when we expand RuntimeHelpers.CreateSpan<int>((RuntimeFieldHandle)token)); in JIT we don't call initClass for the class containing that field. I presume it used to work because CreateSpan wasn't expanded in Tier0 previously and its non-intrinsified path used to properly init it as is, so, presumably, it's #91403 that broke this expansion.

byte RVA works because Roslyn doesn't emit CreateSpan for it (as expected).

@EgorBo
Copy link
Member Author

EgorBo commented Jun 30, 2024

@MihuBot

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

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

@EgorBo EgorBo closed this Jul 1, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 1, 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.

2 participants