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: Convert some of the old-style intrinsics to NamedIntrinsics #52156

Merged
merged 23 commits into from
May 19, 2021

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented May 1, 2021

It's needed for inliner that handles only NamedIntrinsics. Eventually, I guess, we'll get rid of all of the old-style ones.

@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, 2021
@EgorBo EgorBo marked this pull request as ready for review May 4, 2021 07:46
@EgorBo
Copy link
Member Author

EgorBo commented May 4, 2021

@dotnet/jit-contrib PTAL

@echesakov echesakov self-requested a review May 4, 2021 20:53
@EgorBo
Copy link
Member Author

EgorBo commented May 5, 2021

@dotnet/jit-contrib @echesakovMSFT PING 🙂 I'd love to merge it and unblock my next inliner-related PR on top of it

Copy link
Contributor

@echesakov echesakov left a comment

Choose a reason for hiding this comment

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

Sorry for keep you waiting..
Overall, looks good - left some comments.

BTW, have you encountered any diffs with this change?

src/coreclr/jit/gentree.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/valuenum.cpp Outdated Show resolved Hide resolved
@AndyAyersMS
Copy link
Member

Let me echo what @echesakov wrote above: you really need to show that this doesn't lead to diffs. Given the the changes in the jit interface you can't use SPMI -- you should use jit diffs in both PMI and Crossgen2 modes for this, running base and diff separately (since the jits can't be swapped), over all the frameworks assemblies.

@EgorBo
Copy link
Member Author

EgorBo commented May 9, 2021

@AndyAyersMS @echesakovMSFT thanks for review, unfortunately I caught a small regression in crossgen (and crossgen2) so I decided to temporarily leave Object.GetType to be old-style.
Now there are zero size regressions in PMI, crossgen and crossgen2 for corelib and framework libs.

@EgorBo
Copy link
Member Author

EgorBo commented May 10, 2021

So does anybody mind if I merge it (it contains JIT-EE id update)? It's approved and has zero regression across JIT, crossgen and crossgen2

@AndyAyersMS
Copy link
Member

Can you say a bit more about the problems you had with converting Object.GetType and why you ultimately decided you could live without it...perhaps in an issue you open to track converting it to new style?

@EgorBo
Copy link
Member Author

EgorBo commented May 11, 2021

Can you say a bit more about the problems you had with converting Object.GetType and why you ultimately decided you could live without it...perhaps in an issue you open to track converting it to new style?

I didn't give up, just wanted to do it iteratively for all of them eventually to keep the PRs easier to review.

The primary goal is the ability to recognize typeof(T) cmp typeof(T) in inliner (and String.Length), from my understanding - foldable obj.GetType() == typeof() patterns where obj's type is known/final are less common (but I might be wrong).

Object.GetType led to this assert: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/importer.cpp#L8185

eeCallInfo returned CORINFO_VIRTUALCALL_STUB for it instead. I suspect the problem is here: https://github.com/dotnet/runtime/blob/main/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs#L1587-L1589 (and a similar code in the native crossgen1)

Both crossgens know nothing about the new style intrinsics, I tried to fix this specific case like this 65a9154 and looked like it helped but none-windows platforms still asserted so I decided to postpone it a bit 🙁

@EgorBo EgorBo requested a review from marek-safar as a code owner May 11, 2021 16:43
@EgorBo
Copy link
Member Author

EgorBo commented May 11, 2021

@AndyAyersMS so, do you want me to have a look at Object.GetType again as part of this PR or we can have it as is? (I just want to unblock my next PR for inliner)

CI Failures are not related (#52596)

@AndyAyersMS
Copy link
Member

It can be separate. Open an issue and record the details and link it to this PR.

@EgorBo
Copy link
Member Author

EgorBo commented May 11, 2021

It can be separate. Open an issue and record the details and link it to this PR.

Done: #52620

…le-intrins

# Conflicts:
#	src/coreclr/inc/jiteeversionguid.h
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dotnet dotnet deleted a comment from azure-pipelines bot May 18, 2021
@dotnet dotnet deleted a comment from azure-pipelines bot May 18, 2021
@EgorBo EgorBo merged commit 0e1adc0 into dotnet:main May 19, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
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.

None yet

5 participants