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

Expand CORINFO_HELP_CHKCASTANY cast #86728

Merged
merged 2 commits into from May 26, 2023
Merged

Expand CORINFO_HELP_CHKCASTANY cast #86728

merged 2 commits into from May 26, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented May 24, 2023

I am curious if we can expand this helper too, e.g.:

[Benchmark]
public void Test() => GenericCast<string>("");


[MethodImpl(MethodImplOptions.NoInlining)]
public T GenericCast<T>(object o) => (T)o;

Codegen diff for GenericCast. Basically, we use T as a fast-path check (with null check)

; Program.GenericCast[[System.__Canon, System.Private.CoreLib]](System.Object)
       sub       rsp,28
       mov       [rsp+20],rdx
+      mov       rax,r8
+      test      rax,rax
+      je        short M01_L00
       mov       rcx,[rdx+10]
       mov       rcx,[rcx]
+      cmp       [rax],rcx
+      je        short M01_L00
       mov       rdx,r8
       call      [CastHelpers.ChkCastAny(Void*, System.Object)]
       nop
       add       rsp,28
       ret
-; Total bytes of code 31
+; Total bytes of code 44
Method Toolchain Mean Ratio Code Size
Test \Core_Root_PR\corerun.exe 1.120 ns 1.00 80 B
Test \Core_Root_base\corerun.exe 1.500 ns 1.34 67 B

Basically, we inline these things:

image

(Technically, we can introduce a new helper without the fast-paths)

@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 24, 2023
@ghost ghost assigned EgorBo May 24, 2023
@ghost
Copy link

ghost commented May 24, 2023

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

Issue Details

I am curious if we can expand this helper too, e.g.:

[Benchmark]
public void Test() => GenericCast<string>("");


[MethodImpl(MethodImplOptions.NoInlining)]
public T GenericCast<T>(object o) => (T)o;

Codegen diff for GenericCast. Basically, we use runtime lookup as a fast-path check (with null check)

; Program.GenericCast[[System.__Canon, System.Private.CoreLib]](System.Object)
       sub       rsp,28
       mov       [rsp+20],rdx
+      mov       rax,r8
+      test      rax,rax
+      je        short M01_L00
       mov       rcx,[rdx+10]
       mov       rcx,[rcx]
+      cmp       [rax],rcx
+      je        short M01_L00
       mov       rdx,r8
       call      [CastHelpers.ChkCastAny(Void*, System.Object)]
       nop
       add       rsp,28
       ret
-; Total bytes of code 31
+; Total bytes of code 44
Method Toolchain Mean Ratio Code Size
Test \Core_Root_PR\corerun.exe 1.120 ns 1.00 80 B
Test \Core_Root_base\corerun.exe 1.500 ns 1.34 67 B
Author: EgorBo
Assignees: EgorBo
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo EgorBo marked this pull request as ready for review May 24, 2023 23:32
@EgorBo
Copy link
Member Author

EgorBo commented May 24, 2023

@VSadov @jkotas any concerns about it?

@@ -5486,7 +5486,8 @@ GenTree* Compiler::impCastClassOrIsInstToTree(
else if (isCastClass)
{
// Jit can only inline expand CHKCASTCLASS and CHKCASTARRAY helpers.
canExpandInline = (helper == CORINFO_HELP_CHKCASTCLASS) || (helper == CORINFO_HELP_CHKCASTARRAY);
Copy link
Member

Choose a reason for hiding this comment

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

This inlining can be profitable only when the type we are casting to is a class. It is not profitable when the type we are casting to is not a class. For example, it is not profitable when we are casting to IEnumerable<Canon> - it is a pure overhead in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Presumably the overhead is small or can be eliminated completely if we introduce a slightly stripped version of fallback?

I'll check the stats which path is more often taken in a large service like you requested

Copy link
Member

Choose a reason for hiding this comment

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

My primary point was that the inlining of the exact check is not profitable when you can statically tell at JIT time that the target is not a class.

You are right that we can go even further and introduce specialized helper that omits the exact class check. I am not sure whether it is worth it given the overall cost of the helper.

Copy link
Member

Choose a reason for hiding this comment

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

For example, we should not be inlining the exact check in:

[MethodImpl(MethodImplOptions.NoInlining)]
public IEnumrable<T> GenericCast<T>(object o) => (IEnumerable<T>)o;

@jkotas
Copy link
Member

jkotas commented May 25, 2023

It would be useful to have some evidence about how often the fast path is taken vs. not taken in something real world (e.g. Bing).

@EgorBo
Copy link
Member Author

EgorBo commented May 25, 2023

It would be useful to have some evidence about how often the fast path is taken vs. not taken in something real world (e.g. Bing).

@jkotas

I've reverted this PR's change and put these lame counters into the ChkCastAny:

image

Here is the Bing's stats (after running its benchmark):

total=220170000, isNull=117642, isSame=39514246;

so in 18% cases the fast path is taken (0.05% for null)

AvaloniaILSpy desktop app:

total=75000, isNull=14237, isSame=28136;

36% for the fast path.

@jkotas
Copy link
Member

jkotas commented May 25, 2023

Sounds good. These percentages should be even higher once you do the expansion only in places where it is statically known to be profitable.

@EgorBo EgorBo merged commit c7eed31 into dotnet:main May 26, 2023
127 checks passed
@EgorBo EgorBo deleted the expand-castany branch May 26, 2023 09:09
@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 25, 2023
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

2 participants