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

Move GetMethodTable to JIT #105098

Merged
merged 4 commits into from
Aug 19, 2024
Merged

Conversation

MichalPetryka
Copy link
Contributor

Resurrection of #88860 now that the assert is gone since #104976.

Simplifies the VM code and should improve the codegen.

@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 Jul 18, 2024
@MichalPetryka
Copy link
Contributor Author

@MihuBot

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 18, 2024
Copy link
Contributor

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

@EgorBo
Copy link
Member

EgorBo commented Jul 18, 2024

I don't think it's needed in JIT at all, you can just drop the VM side and leave the [Intrinsic] as a hint for the inliner.

@MichalPetryka
Copy link
Contributor Author

I don't think it's needed in JIT at all, you can just drop the VM side and leave the [Intrinsic] as a hint for the inliner.

This should also be safer since we avoid the ref Unsafe.As<RawData>(obj) with this.
Also, gtNewMethodTableLookup spawns the indir as invariant so I'd assume this lets the JIT hoist this better.

@EgorBo
Copy link
Member

EgorBo commented Jul 18, 2024

I don't think it's needed in JIT at all, you can just drop the VM side and leave the [Intrinsic] as a hint for the inliner.

This should also be safer since we avoid the ref Unsafe.As<RawData>(obj) with this. Also, gtNewMethodTableLookup spawns the indir as invariant so I'd assume this lets the JIT hoist this better.

also be safer

we already do a lot of similar casts in BCL, e.g. for arrays.

Also, gtNewMethodTableLookup spawns the indir as invariant so I'd assume this lets the JIT hoist this better.

Jit should be smart enough to figure out that IND<intptr>(ref) is invariant, if it's not - it needs to be fixed then

@JulieLeeMSFT
Copy link
Member

This is optimization, and not correctness issue. We don't have time to work on this for .NET 9, so we will review it in .NET 10.

@jkotas
Copy link
Member

jkotas commented Aug 18, 2024

Jit should be smart enough to figure out that IND(ref) is invariant, if it's not - it needs to be fixed then

Reading the field at offset 0 is tricky. The byref pointing to offset 0 is technically invalid byref. The VM treats is as part of the object currently (vs. as part of the preceding object in memory), but I think we should minimize the number of places that depend on this details. I think it is best to keep reading of the method table as a JIT intrinsic like this PR does. Ideally, I would like to get rid of the m_pEEType field in native AOT to make System.Object impl the same between regular CoreCLR and native AOT.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM

@jkotas
Copy link
Member

jkotas commented Aug 18, 2024

@dotnet/jit-contrib There seems to be an infrastructure issue with SuperPMI. Could you please take a look?

@jakobbotsch
Copy link
Member

/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jkotas jkotas merged commit e96eaee into dotnet:main Aug 19, 2024
106 of 108 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants