Skip to content

NativeAOT: devirtualize isinst/castclass for monomorphic cases - #80831

Merged
EgorBo merged 2 commits into
dotnet:mainfrom
EgorBo:naot-devirt-isinst
Jan 19, 2023
Merged

NativeAOT: devirtualize isinst/castclass for monomorphic cases#80831
EgorBo merged 2 commits into
dotnet:mainfrom
EgorBo:naot-devirt-isinst

Conversation

@EgorBo

Copy link
Copy Markdown
Member

Closes#80828

Luckily, we already have JIT-EE API to get exact list of implementations/subclasses for given type.

interfaceIMyInterface{}// this interface has a single implementationclassProgram:IMyInterface{staticvoidMain(){Case1(newProgram());Case2(newProgram());}[MethodImpl(MethodImplOptions.NoInlining)]staticboolCase1(objecto)=>oisIMyInterface;[MethodImpl(MethodImplOptions.NoInlining)]staticIMyInterfaceCase2(objecto)=>(IMyInterface)o;}

Previous NativeAOT codegen:

; Method Program:Case1(System.Object):boolsubrsp,40movrdx,rcxlearcx,[(reloc)]call CORINFO_HELP_ISINSTANCEOFINTERFACEtestrax,rax setne almovzxrax,aladdrsp,40ret; Total bytes of code: 33; Method Program:Case2(System.Object):IMyInterfacesubrsp,40movrdx,rcxlearcx,[(reloc)]call CORINFO_HELP_CHKCASTINTERFACEnopaddrsp,40ret; Total bytes of code: 25

New NativeAOT codegen:

; Method Program:Case1(System.Object):booltestrcx,rcxje SHORT G_M13626_IG05learax,[(reloc)]cmp qword ptr [rcx],raxje SHORT G_M13626_IG05xorrcx,rcxG_M13626_IG05:xoreax,eaxtestrcx,rcx setne alret; Total bytes of code: 28; Method Program:Case2(System.Object):IMyInterfacesubrsp,40movrdx,rcxmovrax,rdxtestrax,raxje SHORT G_M12806_IG05learcx,[(reloc)]cmp qword ptr [rax],rcxje SHORT G_M12806_IG05movrcx, qword ptr [rsp+20H]call CORINFO_HELP_CHKCASTINTERFACE ;; this is used to throw InvalidCastException onlyG_M12806_IG05:nopaddrsp,40ret; Total bytes of code: 43

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 19, 2023
@ghostghost assigned EgorBoJan 19, 2023
@ghost

Copy link
Copy Markdown

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

Issue Details

Closes #80828

Luckily, we already have JIT-EE API to get exact list of implementations/subclasses for given type.

interfaceIMyInterface{}// this interface has a single implementationclassProgram:IMyInterface{staticvoidMain(){Case1(newProgram());Case2(newProgram());}[MethodImpl(MethodImplOptions.NoInlining)]staticboolCase1(objecto)=>oisIMyInterface;[MethodImpl(MethodImplOptions.NoInlining)]staticIMyInterfaceCase2(objecto)=>(IMyInterface)o;}

Previous NativeAOT codegen:

; Method Program:Case1(System.Object):boolsubrsp,40movrdx,rcxlearcx,[(reloc)]call CORINFO_HELP_ISINSTANCEOFINTERFACEtestrax,rax setne almovzxrax,aladdrsp,40ret; Total bytes of code: 33; Method Program:Case2(System.Object):IMyInterfacesubrsp,40movrdx,rcxlearcx,[(reloc)]call CORINFO_HELP_CHKCASTINTERFACEnopaddrsp,40ret; Total bytes of code: 25

New NativeAOT codegen:

; Method Program:Case1(System.Object):booltestrcx,rcxje SHORT G_M13626_IG05learax,[(reloc)]cmp qword ptr [rcx],raxje SHORT G_M13626_IG05xorrcx,rcxG_M13626_IG05:xoreax,eaxtestrcx,rcx setne alret; Total bytes of code: 28; Method Program:Case2(System.Object):IMyInterfacesubrsp,40movrdx,rcxmovrax,rdxtestrax,raxje SHORT G_M12806_IG05learcx,[(reloc)]cmp qword ptr [rax],rcxje SHORT G_M12806_IG05movrcx, qword ptr [rsp+20H]call CORINFO_HELP_CHKCASTINTERFACE ;; this is used to throw InvalidCastException onlyG_M12806_IG05:nopaddrsp,40ret; Total bytes of code: 43
Author:EgorBo
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@am11

am11 commented Jan 19, 2023

Copy link
Copy Markdown
Member

Is Case2 regression avoidable?

@AndyAyersMS

Copy link
Copy Markdown
Member

Is Case2 regression avoidable?

The code is bigger but should also be faster, since we will rarely need to call the helper (only if the cast will throw).

Though I'm not quite sure why we're shuffling those registers about like we do...

@MichalStrehovsky

Copy link
Copy Markdown
Member

Curious: if we had hot/cold splitting support in NativeAOT (#77583), would that place the ;; this is used to throw InvalidCastException only in the cold region, or would that be more work?

Comment on lines +5720 to +5723
if (this->IsTargetAbi(CORINFO_NATIVEAOT_ABI) &&
((helper == CORINFO_HELP_ISINSTANCEOFINTERFACE) || (helper == CORINFO_HELP_CHKCASTINTERFACE)) &&
(info.compCompHnd->getExactClasses(pResolvedToken->hClass, 1, &actualImplCls) == 1) &&
(actualImplCls != NO_CLASS_HANDLE) && impIsClassExact(actualImplCls))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any reason this optimization shouldn't kick in for any other potential EE that returns 1 from getExactClasses?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jakobbotsch purely throughput reasons - 2 ifs + non inlined call getExactClasses since I don't think it will be useful outside of NativeAOT anytime soon 🙂

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems a bit premature? If I remove the NAOT check and hack SPMI to always return 0 on replay of getExactClasses I see no significant TP differences. IMO, as the general rule (no need to change this now) we should not be sacrificing abstractions unless they have demonstrable impact.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

no strong opinion on that, will remove as part of some next PR

@EgorBo

Copy link
Copy Markdown
MemberAuthor

Curious: if we had hot/cold splitting support in NativeAOT (#77583), would that place the ;; this is used to throw InvalidCastException only in the cold region, or would that be more work?

It doesn't look like it's handled in hot/cold splitting now, but at least it's still considered as "cold" in jit

@EgorBo
EgorBo merged commit 4544bdd into dotnet:mainJan 19, 2023
@EgorBo
EgorBo deleted the naot-devirt-isinst branch January 19, 2023 12:32
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
@ghostghost locked as resolved and limited conversation to collaborators Feb 18, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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.

NativeAOT: devirtualize isinst for monomorphic cases

5 participants

@EgorBo@am11@AndyAyersMS@MichalStrehovsky@jakobbotsch