Optimize GDV checks for objects with known type #84661

EgorBo · 2023-04-11T22:19:34Z

Optimize *obj == const_class GDV's guards if we can get the exact type of obj. This is needed for #84659

Codegen improvement example:

string Test(byte[] data) => Encoding.UTF8.GetString(data);

Was:

; Assembly listing for method Prog:Test(ubyte[]):System.String:this
; Tier-1 compilation
; 1 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data
       sub      rsp, 40
       mov      rcx, 0x1C4C0C009F0      ; const ptr
       mov      rcx, gword ptr [rcx]
       mov      rax, 0x7FFE3DD8A348      ; System.Text.UTF8Encoding+UTF8EncodingSealed
       cmp      qword ptr [rcx], rax ;;; <---- redundant check, obj is already of UTF8EncodingSealed
       jne      SHORT G_M52604_IG07
       test     rdx, rdx
       je       SHORT G_M52604_IG04
       cmp      dword ptr [rdx+08H], 32
       jg       SHORT G_M52604_IG04
       call     [System.Text.UTF8Encoding+UTF8EncodingSealed:GetStringForSmallInput(ubyte[]):System.String:this]
       jmp      SHORT G_M52604_IG05
G_M52604_IG04:  
       call     [System.Text.Encoding:GetString(ubyte[]):System.String:this]
G_M52604_IG05:  
       nop      
       add      rsp, 40
       ret      
G_M52604_IG07:  
       call     [System.Text.UTF8Encoding+UTF8EncodingSealed:GetString(ubyte[]):System.String:this]
       jmp      SHORT G_M52604_IG05
; Total bytes of code 71

Now:

; Assembly listing for method Prog:Test(ubyte[]):System.String:this
; Tier-1 compilation
; optimized code
; 1 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data
       sub      rsp, 40
       mov      rcx, 0x1B4148009F0      
       mov      rcx, gword ptr [rcx]
       test     rdx, rdx
       je       SHORT G_M52604_IG04
       cmp      dword ptr [rdx+08H], 32
       jg       SHORT G_M52604_IG04
       call     [System.Text.UTF8Encoding+UTF8EncodingSealed:GetStringForSmallInput(ubyte[]):System.String:this]
       jmp      SHORT G_M52604_IG05
G_M52604_IG04:  
       call     [System.Text.Encoding:GetString(ubyte[]):System.String:this]
G_M52604_IG05:  
       nop      
       add      rsp, 40
       ret      
; Total bytes of code 48

ghost · 2023-04-11T22:19:46Z

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

Issue Details

Optimize *obj == const_class GDV's guards if we can get the exact type of obj. This is needed for #84659

Codegen improvement example:

string Test(byte[] data) => Encoding.UTF8.GetString(data);

Was:

; Assembly listing for method Prog:Test(ubyte[]):System.String:this
; Tier-1 compilation
; 1 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data
       sub      rsp, 40
       mov      rcx, 0x1C4C0C009F0      ; const ptr
       mov      rcx, gword ptr [rcx]
       mov      rax, 0x7FFE3DD8A348      ; System.Text.UTF8Encoding+UTF8EncodingSealed
       cmp      qword ptr [rcx], rax ;;; <---- redundant check, obj is already of UTF8EncodingSealed
       jne      SHORT G_M52604_IG07
       test     rdx, rdx
       je       SHORT G_M52604_IG04
       cmp      dword ptr [rdx+08H], 32
       jg       SHORT G_M52604_IG04
       call     [System.Text.UTF8Encoding+UTF8EncodingSealed:GetStringForSmallInput(ubyte[]):System.String:this]
       jmp      SHORT G_M52604_IG05
G_M52604_IG04:  
       call     [System.Text.Encoding:GetString(ubyte[]):System.String:this]
G_M52604_IG05:  
       nop      
       add      rsp, 40
       ret      
G_M52604_IG07:  
       call     [System.Text.UTF8Encoding+UTF8EncodingSealed:GetString(ubyte[]):System.String:this]
       jmp      SHORT G_M52604_IG05
; Total bytes of code 71

Now:

; Assembly listing for method Prog:Test(ubyte[]):System.String:this
; Tier-1 compilation
; optimized code
; 1 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data
       sub      rsp, 40
       mov      rcx, 0x1B4148009F0      
       mov      rcx, gword ptr [rcx]
       test     rdx, rdx
       je       SHORT G_M52604_IG04
       cmp      dword ptr [rdx+08H], 32
       jg       SHORT G_M52604_IG04
       call     [System.Text.UTF8Encoding+UTF8EncodingSealed:GetStringForSmallInput(ubyte[]):System.String:this]
       jmp      SHORT G_M52604_IG05
G_M52604_IG04:  
       call     [System.Text.Encoding:GetString(ubyte[]):System.String:this]
G_M52604_IG05:  
       nop      
       add      rsp, 40
       ret      
; Total bytes of code 48

Author:	EgorBo
Assignees:	EgorBo
Labels:	`area-CodeGen-coreclr`
Milestone:	-

EgorBo · 2023-04-11T22:20:04Z

PTAL @AndyAyersMS - I wanted to land this fix in the past and gave up but now I found a real use-case

AndyAyersMS · 2023-04-11T22:46:07Z

Seems like something we already should have been able to clean up.

What is it we know when we do this new type compare fold that we don't know later on (say in value numbering)?

EgorBo · 2023-04-11T22:55:41Z

Seems like something we already should have been able to clean up.

What is it we know when we do this new type compare fold that we don't know later on (say in value numbering)?

I don't think we have anything like that in VN, you probably confuse it with your #72136 which is different. I could move this opt to VN as well but all the cases I found looked like can be handled earlier in Morph. Essentially, it happens when we have something like this:

Foo().Bar();

when we look at Bar virtual call we mark it as GDV candidate because we don't know exact type of Foo(). Then later when we inline Foo() and get something with exact type, so we ended up with that redundant GDV check. So it's a sort of inlining-ordering issue that we clean up here.

If you want to repro it locally:

public class ClassA
{
    public virtual int GetVal() => 42;
}

public sealed class ClassB : ClassA
{
    public override int GetVal() => 43;
}

class Prog
{
    static readonly ClassB s_default = new ClassB();

    static ClassA Default => s_default;

    static void Main()
    {
        for (int i = 0; i < 100; i++)
        {
            Test();
            Thread.Sleep(16);
        }
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Test() => Default.GetVal();
}

Run with

DOTNET_TieredPGO=1
DOTNET_JitDisasm=Test

It simulates a similar pattern I found in Encoding.UTF8.*

AndyAyersMS · 2023-04-11T23:20:36Z

With your changes is this firing in LateDevirtualization?

I don't think we have anything like that in VN, you probably confuse it with your #72136 which is different.

Is what you're saying is that VN doesn't know how to handle method table loads from these "const ptrs"?

I'm ok fixing it in morph but remember that morph is purely local and if a known value arrives at the comparison via some data flow morph will never see it. So consider fixing it in VN as well.

EgorBo · 2023-04-11T23:25:58Z

@AndyAyersMS I've just merged VN impl 🙂 It turned out VN knew it only if it could find VNF_JitNew func, so basically it used track allocations only

EgorBo · 2023-04-11T23:44:07Z

It seems it found quite a few cases for my app running locally with PGO.

AndyAyersMS

LGTM, but I am ok also having it checked earlier too, like you were doing.

Usually, the earlier the better for these things.

EgorBo · 2023-04-12T13:54:48Z

Some nice diffs in benchmarks.run.pgo collection (thanks for adding it!). We don't have apsnet one but presumably it should also hit them. I didn't see any diff improvements from the change in morph so I removed it.

Although, it seems that NativeAOT is not happy. Perhaps, it's not legal to represent types as constants there?

AndyAyersMS · 2023-04-12T14:44:16Z

I just redid the ASP.NET collection but maybe another GUID change came in that I missed. Will start it going again.

EgorBo · 2023-04-12T16:41:12Z

/azp list

EgorBo · 2023-04-12T16:41:51Z

/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo

azure-pipelines · 2023-04-12T16:42:17Z

Azure Pipelines successfully started running 2 pipeline(s).

AndyAyersMS · 2023-04-12T17:51:51Z

I just redid the ASP.NET collection but maybe another GUID change came in that I missed. Will start it going again.

The new collection just uploaded.

EgorBo · 2023-04-12T18:08:44Z

I just redid the ASP.NET collection but maybe another GUID change came in that I missed. Will start it going again.

The new collection just uploaded.

Thanks! -15237 bytes on that one.

EgorBo · 2023-04-12T18:09:05Z

/azp run runtime-extra-platforms

EgorBo · 2023-04-14T16:08:50Z

/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo

azure-pipelines · 2023-04-14T16:09:09Z

Azure Pipelines successfully started running 2 pipeline(s).

SingleAccretion · 2023-04-14T16:28:06Z

src/coreclr/jit/valuenum.cpp

+                        void* embedClsHnd = (void*)info.compCompHnd->embedClassHandle(handle, &pEmbedClsHnd);
+                        if (pEmbedClsHnd == nullptr)
+                        {
+                            // Skip indirect handles for now since this path is mostly for PGO scenarios


Note: indirect handles could be handled by essentially switching out the address for the indirection to be pEmbedClsHnd.

src/coreclr/jit/valuenum.cpp

Co-authored-by: Jan Kotas <[email protected]>

EgorBo · 2023-04-21T20:22:46Z

I'm struggling with a NativeAOT assert (happens when ILC compiles crossgen2 project) - it happens inside embedClassHandle EE API Jit calls:

where the values in the assert are:

{[S.P.CoreLib]System.Action`1<System.Collections.Generic.List`1<ILCompiler.DependencyAnalysisFramework.DependencyNodeCore`1<System.__Canon>>>}

==

{[S.P.CoreLib]System.Action`1<System.__Canon>}

@MichalStrehovsky @jkotas does it tell you anything, Am I even allowed to use shared types at all in that API?

I've inserted info.compCompHnd->compareTypesForEquality(handle, handle) != TypeCompareState::May check to filter shared types out and it fixed the assert but the jit diffs are now empty 🙁

jkotas · 2023-04-21T20:37:01Z

The assert is complaining about directly embedding type handle that requires dictionary lookup. It is not right in any configs. The types that require dictionary lookup should be never embedded directly. You can try to check CORINFO_FLG_SHAREDINST to see whether the type requires dictionary lookup.

EgorBo · 2023-04-21T20:46:29Z

The assert is complaining about directly embedding type handle that requires dictionary lookup. It is not right in any configs. The types that require dictionary lookup should be never embedded directly. You can try to check CORINFO_FLG_SHAREDINST to see whether the type requires dictionary lookup.

Thanks, that makes perfect sense

EgorBo · 2023-04-21T22:27:59Z

/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo

azure-pipelines · 2023-04-21T22:28:18Z

Azure Pipelines successfully started running 2 pipeline(s).

EgorBo · 2023-04-22T00:19:00Z

The check has fixed the assert and the diffs are back to normal: https://dev.azure.com/dnceng-public/public/_build/results?buildId=248263&view=ms.vss-build-web.run-extensions-tab

EgorBo · 2023-04-22T10:23:30Z

all CI jobs passed except license/CLA 😢

Optimize GDV check

f31dd26

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 11, 2023

ghost assigned EgorBo Apr 11, 2023

Improve

7a754d6

move to VN

0246f11

use handle

3e5d5d6

AndyAyersMS approved these changes Apr 11, 2023

View reviewed changes

EgorBo added 6 commits April 12, 2023 11:52

Add in Morph too

4934611

Merge branch 'main' of github.com:dotnet/runtime into improve-gdv-check

ef2ef49

add comments

4446e6d

Update morph.cpp

846e7d7

fix build

349e77a

remove morph part

036f431

This comment was marked as off-topic.

Sign in to view

Merge branch 'main' of github.com:dotnet/runtime into improve-gdv-check

67aa3e5

EgorBo added 3 commits April 14, 2023 14:36

fix cg?

2477124

Merge branch 'main' of github.com:dotnet/runtime into improve-gdv-check

e55663c

skip indirect handles

70e5e38

SingleAccretion approved these changes Apr 14, 2023

View reviewed changes

Address feedback

3b18809

jkotas reviewed Apr 14, 2023

View reviewed changes

src/coreclr/jit/valuenum.cpp Outdated Show resolved Hide resolved

Update src/coreclr/jit/valuenum.cpp

602a2ba

Co-authored-by: Jan Kotas <[email protected]>

EgorBo mentioned this pull request Apr 15, 2023

Fold GDV checks for inlined delegates #84872

Closed

EgorBo added 2 commits April 21, 2023 16:47

Merge branch 'main' of github.com:dotnet/runtime into improve-gdv-check

737b3a4

fix naot assert

beb3ab9

EgorBo force-pushed the improve-gdv-check branch from c0bdf20 to beb3ab9 Compare April 21, 2023 16:27

build-analysis bot mentioned this pull request Apr 21, 2023

Could not load file or assembly 'Microsoft.CodeAnalysis.NetAnalyzers #84995

Closed

Apply Jan's suggestion

166e900

build-analysis bot mentioned this pull request Apr 22, 2023

System.Net.Quic.Tests.QuicStreamTests.WriteCanceled_NextWriteThrows test failure #76831

Closed

EgorBo closed this Apr 22, 2023

EgorBo reopened this Apr 22, 2023

EgorBo merged commit 7c61b98 into dotnet:main Apr 22, 2023

EgorBo deleted the improve-gdv-check branch April 22, 2023 13:14

EgorBo mentioned this pull request Apr 23, 2023

Inliner: don't give up on virtual calls if we can clarify their exact targets after inlining #85209

Open

ghost locked as resolved and limited conversation to collaborators May 22, 2023

Optimize GDV checks for objects with known type #84661

Optimize GDV checks for objects with known type #84661

Uh oh!

Conversation

EgorBo commented Apr 11, 2023

Uh oh!

ghost commented Apr 11, 2023

Uh oh!

EgorBo commented Apr 11, 2023

Uh oh!

AndyAyersMS commented Apr 11, 2023

Uh oh!

EgorBo commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndyAyersMS commented Apr 11, 2023

Uh oh!

EgorBo commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EgorBo commented Apr 11, 2023

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndyAyersMS commented Apr 12, 2023

Uh oh!

EgorBo commented Apr 12, 2023

Uh oh!

This comment was marked as off-topic.

EgorBo commented Apr 12, 2023

Uh oh!

azure-pipelines bot commented Apr 12, 2023

Uh oh!

AndyAyersMS commented Apr 12, 2023

Uh oh!

EgorBo commented Apr 12, 2023

Uh oh!

EgorBo commented Apr 12, 2023

Uh oh!

EgorBo commented Apr 14, 2023

Uh oh!

azure-pipelines bot commented Apr 14, 2023

Uh oh!

SingleAccretion Apr 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EgorBo commented Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Apr 21, 2023

Uh oh!

EgorBo commented Apr 21, 2023

Uh oh!

EgorBo commented Apr 21, 2023

Uh oh!

azure-pipelines bot commented Apr 21, 2023

Uh oh!

EgorBo commented Apr 22, 2023

Uh oh!

EgorBo commented Apr 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

EgorBo commented Apr 11, 2023 •

edited

Loading

EgorBo commented Apr 11, 2023 •

edited

Loading

EgorBo commented Apr 12, 2023 •

edited

Loading

EgorBo commented Apr 21, 2023 •

edited

Loading