Skip to content

JIT: Use metadata names for SIMD intrinsic method recognition#76786

Merged
jakobbotsch merged 8 commits intodotnet:mainfrom
jakobbotsch:avoid-eeGetMethodName
Oct 22, 2022
Merged

JIT: Use metadata names for SIMD intrinsic method recognition#76786
jakobbotsch merged 8 commits intodotnet:mainfrom
jakobbotsch:avoid-eeGetMethodName

Conversation

@jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Oct 8, 2022 •
edited
Loading

eeGetMethodName is only expected to be used for diagnostics.

In practice this does not cause issues, but some odd situations could occur in the cases where eeGetMethodName returns a placeholder name during SPMI replay.

Also avoid unnecessary EE call in impSIMDIntrinsic in the common case of no SIMD intrinsic.

@jakobbotsch
JIT: Use metadata names for SIMD intrinsic method recognition
40426f0 
eeGetMethodName is only expected to be used for diagnostics.
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 8, 2022
@ghost ghost assigned jakobbotsch Oct 8, 2022
@ghost
Copy link

ghost commented Oct 8, 2022

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

Issue Details

eeGetMethodName is only expected to be used for diagnostics.

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@jkotas
Copy link
Member

jkotas commented Oct 8, 2022

If I am reading the code correctly, we are always going to ask for getMethodNameFromMetadata twice for SIMD methods. First time during attempt to import non-SIMD intrinsic and second time here. Would it be worth it to avoid these duplicate calls?

@jkotas
Copy link
Member

jkotas commented Oct 8, 2022 •
edited
Loading

If it is too complex refactoring, we should add if (isIntrinsic) condition around the following in importer.cpp:

    {
        call = impSIMDIntrinsic(opcode, newobjThis, clsHnd, methHnd, sig, mflags, pResolvedToken->token);
        if (call != nullptr)
        {
            bIntrinsicImported = true;
            goto DONE_CALL;
        }

and flip the same check in impSIMDIntrinsic to assert. It will avoid the more expensive isSIMDClass checks for nearly all methods calls.

@jakobbotsch
Copy link
Member Author

jakobbotsch commented Oct 8, 2022

It's a good point. The refactoring does look a little complicated to do, but impImportCall is long overdue for a refactor so maybe I can spend some time on that. I also noticed that we needlessly query the signature for all hardware intrinsics even though we already have it from the call info.

@jakobbotsch
Copy link
Member Author

jakobbotsch commented Oct 9, 2022

@jkotas I started with a larger refactoring but as you mentioned it does end up being a more major operation, so for this PR I went with your second suggestion instead.

jakobbotsch
jakobbotsch commented Oct 9, 2022
View reviewed changes
src/coreclr/vm/jitinterface.cpp
if (enclosingClassName != NULL && pMT->GetClass()->IsNested())
{
IfFailThrow(pMDImport->GetNameOfTypeDef(pMT->GetEnclosingCl(), &enclosingResult, &namespaceResult));
IfFailThrow(pMDImport->GetNameOfTypeDef(pMT->GetEnclosingCl(), enclosingClassName, namespaceName));
Copy link
Member Author

@jakobbotsch jakobbotsch Oct 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This logic seems a bit odd to me in terms of the namespaceResult, IIUC it will work only for one level of nesting.

Copy link
Member

@jkotas jkotas Oct 9, 2022

Choose a reason for hiding this comment

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

This is designed to handle just enough for the JIT to identify intrinsics. We only need one level of nesting for that.

This was referenced Oct 10, 2022
Closed
Closed
Closed
@jakobbotsch jakobbotsch reopened this Oct 10, 2022
tannergooding
tannergooding reviewed Oct 10, 2022
View reviewed changes
src/coreclr/jit/importer.cpp Outdated
{
bIntrinsicImported = true;
goto DONE_CALL;
call = impSIMDIntrinsic(opcode, newobjThis, clsHnd, methHnd, sig, mflags, pResolvedToken->token);
Copy link
Member

@tannergooding tannergooding Oct 10, 2022

Choose a reason for hiding this comment

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

Should this just be part of the slightly higher check that already exists?

There isn't any real reason it should be separate or not handled in impIntrinsic...

Copy link
Member Author

@jakobbotsch jakobbotsch Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I initially tried refactoring this and moving it in to be part of impIntrinsic, but it turned out to be a bit more work than I wanted here. Also wasn't totally sure with the alt-jit check if I could just move it under the existing if.

I am planning to do some more refactoring for this (starting with #76792) so I think I will leave the further clean up to a future PR.

@build-analysis build-analysis bot mentioned this pull request Oct 11, 2022
Closed
2 tasks
@build-analysis build-analysis bot mentioned this pull request Oct 11, 2022
Closed
2 tasks
@jakobbotsch
Copy link
Member Author

jakobbotsch commented Oct 22, 2022

Failure is known according to build analysis.

@jakobbotsch jakobbotsch merged commit 3f40c3d into dotnet:main Oct 22, 2022
@jakobbotsch jakobbotsch deleted the avoid-eeGetMethodName branch October 22, 2022 11:45
@ghost ghost locked as resolved and limited conversation to collaborators Nov 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tannergooding tannergooding tannergooding left review comments

@jkotas jkotas jkotas approved these changes

Assignees

@jakobbotsch jakobbotsch

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jakobbotsch @jkotas @tannergooding