JIT: Expand runtime lookups in a late phase #81635

EgorBo · 2023-02-04T13:19:39Z

Closes #35551 and is a preparation for #81432

The main advantage of the late expansion is that we will be able to do CSE/Hoisting for such helpers, e.g.:

static void Test<T>()
{
    for (int i = 0; i < 100; i++)
        Callee<T>();
}

Current codegen:

; Assembly listing for method Prog:Test[System.__Canon]()
       57                   push     rdi
       56                   push     rsi
       53                   push     rbx
       4883EC30             sub      rsp, 48
       48894C2428           mov      qword ptr [rsp+28H], rcx
       488BF1               mov      rsi, rcx
       33FF                 xor      edi, edi
       488B5E10             mov      rbx, qword ptr [rsi+10H]

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; loop start
G_M000_IG03:                
       488B4B10             mov      rcx, qword ptr [rbx+10H]
       4885C9               test     rcx, rcx
       7402                 je       SHORT G_M000_IG05
       EB15                 jmp      SHORT G_M000_IG06
G_M000_IG05:               
       488BCE               mov      rcx, rsi
       48BAB80C584EFC7F0000 mov      rdx, 0x7FFC4E580CB8
       E87E36A95F           call     CORINFO_HELP_RUNTIMEHANDLE_METHOD
       488BC8               mov      rcx, rax
G_M000_IG06:                
       FF159D932800         call     [Prog:Callee[System.__Canon]():bool]
       FFC7                 inc      edi
       83FF64               cmp      edi, 100
       7CD3                 jl       SHORT G_M000_IG03
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; loop end

       4883C430             add      rsp, 48
       5B                   pop      rbx
       5E                   pop      rsi
       5F                   pop      rdi
       C3                   ret      
; Total bytes of code 74

We were not able to hoist the runtime lookup stuff out of loop while this PR fixes that.
New codegen:

; Assembly listing for method Prog:Test[System.__Canon]()
G_M2205_IG01:              
       57                   push     rdi
       56                   push     rsi
       53                   push     rbx
       4883EC30             sub      rsp, 48
       48894C2428           mov      qword ptr [rsp+28H], rcx
G_M2205_IG02:              
       33F6                 xor      esi, esi
       488B5138             mov      rdx, qword ptr [rcx+38H]
       488B7A10             mov      rdi, qword ptr [rdx+10H]
       4885FF               test     rdi, rdi
       7405                 je       SHORT G_M2205_IG04
G_M2205_IG03:              
       488BDF               mov      rbx, rdi
       EB12                 jmp      SHORT G_M2205_IG05
G_M2205_IG04:              
       48BA20D3D3B5FC7F0000 mov      rdx, 0x7FFCB5D3D320      ; global ptr
       E8A1D10F5F           call     CORINFO_HELP_RUNTIMEHANDLE_METHOD
       488BD8               mov      rbx, rax
       
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; loop start
G_M2205_IG05:              
       488BCB               mov      rcx, rbx
       FF1595216000         call     [Prog:Callee[System.__Canon]()]
       FFC6                 inc      esi
       83FE64               cmp      esi, 100
       7CF0                 jl       SHORT G_M2205_IG05
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; loop end

G_M2205_IG06:              
       4883C430             add      rsp, 48
       5B                   pop      rbx
       5E                   pop      rsi
       5F                   pop      rdi
       C3                   ret      
; Total bytes of code 74

The call is correctly hoisted

This is also required to do proper fix for @jkotas's suggestion in #81432 (comment)

Quite nice diffs

ghost · 2023-02-04T13:19:51Z

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

Issue Details

Closes #35551 and is a preparation for #81432

The main advantage of the late expansion is that we will be able to do CSE/Hoisting for such helpers, e.g.:

static void Test<T>()
{
    for (int i = 0; i < 100; i++)
        Callee<T>();
}

Current codegen:

; Assembly listing for method Prog:Test[System.__Canon]()
       57                   push     rdi
       56                   push     rsi
       53                   push     rbx
       4883EC30             sub      rsp, 48
       48894C2428           mov      qword ptr [rsp+28H], rcx
       488BF1               mov      rsi, rcx
       33FF                 xor      edi, edi
       488B5E10             mov      rbx, qword ptr [rsi+10H]

       ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; loop start
G_M000_IG03:                
       488B4B10             mov      rcx, qword ptr [rbx+10H]
       4885C9               test     rcx, rcx
       7402                 je       SHORT G_M000_IG05
       EB15                 jmp      SHORT G_M000_IG06
G_M000_IG05:               
       488BCE               mov      rcx, rsi
       48BAB80C584EFC7F0000 mov      rdx, 0x7FFC4E580CB8
       E87E36A95F           call     CORINFO_HELP_RUNTIMEHANDLE_METHOD
       488BC8               mov      rcx, rax
G_M000_IG06:                
       FF159D932800         call     [Prog:Callee[System.__Canon]():bool]
       FFC7                 inc      edi
       83FF64               cmp      edi, 100
       7CD3                 jl       SHORT G_M000_IG03
       ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; loop end

       4883C430             add      rsp, 48
       5B                   pop      rbx
       5E                   pop      rsi
       5F                   pop      rdi
       C3                   ret      
; Total bytes of code 74

We were not able to hoist the runtime lookup stuff out of loop while this PR fixes that.
This is also required to do proper fix for @jkotas's suggestion in #81432 (comment)

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

src/coreclr/jit/indirectcalltransformer.cpp

…lookups-later

AndyAyersMS · 2023-02-06T15:57:20Z

Presumably, the dynamically expandable ones can't be marked as "pure"

Right, they are not pure.

jakobbotsch · 2023-02-06T16:03:18Z

They might not be "pure" for a very strict sense of pure, but like the comment says, can they not be optimized as such from the JIT's perspective? I.e. they are idempotent.

AndyAyersMS · 2023-02-06T16:29:29Z

Well, there are (were?) issues like #40298. I vaguely recall some other problem but haven't pinned it down yet.

AndyAyersMS · 2023-02-06T16:36:23Z

Well, there are (were?) issues like #40298. I vaguely recall some other problem but haven't pinned it down yet.

Hmm. Maybe the other thing was just making sure we don't mark some of those indirs in the early expansions as invariant.

jkotas · 2023-02-06T17:44:22Z

The dictionary lookup as a whole is pure. The helper call to populate the dictionary slot lazily is not pure (ie it has observable side-effects).

…lookups-later

EgorBo · 2023-02-25T17:33:43Z

Fun fact about this PR - non-inlined runtime lookups lead to test timeouts. Initially I suspected some freezes/crashes but it turned out the code is just slow 😐

E.g. System.ObjectModel.Tests takes around a second to finish normally and 1 minutes without expanded runtime lookups

This reverts commit 22f34a1.

src/coreclr/jit/runtimelookup.cpp

Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>

EgorBo · 2023-03-14T18:25:12Z

Anything else? New diffs - TP is even better than previously for Tier0, thanks @jakobbotsch

jakobbotsch

LGTM!

AndyAyersMS

LGTM, just a few questions....

AndyAyersMS · 2023-03-14T19:58:05Z

src/coreclr/jit/fgbasic.cpp

+ BasicBlockFlags originalFlags = block->bbFlags;
+ BasicBlock* prevBb = block;
+
+ if (stmt == block->firstStmt())


This is checking for the case where gtSplitTree didn't do anything?

Perhaps deserves a comment to that effect.

I'll add a comment in a follow up if you don't mind to avoid re-running CI for it

Is this really about gtSplitTree? Isn't it here because we don't have a fgSplitBlockBefore?

gtSplitTree can make changes even without introducing new statements -- the return value needs to be used for that kind of check.

Yes, it's here just because I need basically fgSplitBlockBefore and only have fgSplitBlockAfter API

so when it's not the first statement in the current block I do fgSplitBlockAfter (stmt->PrevStmt())

I suppose I should look at what gtSplitTree does, but I'm still confused exactly what this method is supposed to be doing. Maybe an example would help?

I suppose I should look at what gtSplitTree does, but I'm still confused exactly what this method is supposed to be doing. Maybe an example would help?

The general idea that we want to split a block (e.g. BB0) into two (say, BBa and BBb) at a specific node (e.g. callX) and make sure that all side-effects are moved to BBa and the actual callX is now in BBb.

We have a phase where we insert GC safe points after specific call nodes - that one didn't have to care about any execution ordering since we just wanted to make sure GC is polled (it doesn't even matter whether to emit the poll before or after the calls). The runtime lookup case is a lot more complicated since we needed to insert verbose tree in front of the call and re-use its arguments, respect all kinds of complex COMMAs,

AndyAyersMS · 2023-03-14T20:05:10Z

src/coreclr/jit/importer.cpp


-  assert((lastIndOfTree != nullptr) && (pRuntimeLookup->indirections > 0));
+ impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("bubbling QMark0"));


Is there any benefit to deferring expansion of the rest of this in a manner similar to the case above? Seems like this is also creating a hoistable/cseable complex.

I didn't touch this path because it wasn't clear for me when it's hit. From what I see it's not used by R2R and NAOT. Perhaps some dynamic context

In a follow I'll see if it's worth supporting

testForFixup was used by fragile NGen. It is not used anymore. I think it is fine to delete it - both on the JIT/EE interface and the supporting code in the JIT.

testForFixup was used by fragile NGen. It is not used anymore. I think it is fine to delete it - both on the JIT/EE interface and the supporting code in the JIT.

Thanks, will delete in #83430

EgorBo · 2023-03-21T12:52:07Z

Some CSE/Hoisting related Improvements on x64:

[Perf] Alpine/x64: 36 Improvements on 3/15/2023 12:03:45 AM perf-autofiling-issues#14217
[Perf] Linux/x64: 32 Improvements on 3/15/2023 12:03:45 AM perf-autofiling-issues#14211
[Perf] Windows/x86: 47 Improvements on 3/15/2023 12:03:45 AM perf-autofiling-issues#14244
[Perf] Windows/x64: 42 Improvements on 3/15/2023 12:03:45 AM perf-autofiling-issues#14277

Expand runtime lookups in a late phase

2dcd5e4

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 4, 2023

ghost assigned EgorBo Feb 4, 2023

EgorBo commented Feb 4, 2023

View reviewed changes

src/coreclr/jit/indirectcalltransformer.cpp Show resolved Hide resolved

Clean up

2c7a4a3

This was referenced Feb 4, 2023

System.Text.RegularExpressions.Tests.AttRegexTests test fails in CI #75808

Closed

Test failure Loader\\classloader\\DictionaryExpansion\\DictionaryExpansion\\DictionaryExpansion.cmd #75244

Closed

jakobbotsch mentioned this pull request Feb 5, 2023

SPMI: Fix SpmiRecordsHelper::RestoreCORINFO_RUNTIME_LOOKUP #81657

Merged

Merge branch 'main' of github.com:dotnet/runtime into expand-runtime-…

5754331

…lookups-later

EgorBo mentioned this pull request Feb 6, 2023

Don't clone a large tree in impRuntimeLookupToTree #81472

Closed

test

dca8140

This comment was marked as outdated.

Sign in to view

test2

e303869

runfoapp bot mentioned this pull request Feb 6, 2023

Infra improvements for Helix #68176

Closed

EgorBo added 4 commits February 25, 2023 11:29

Merge branch 'main' of github.com:dotnet/runtime into expand-runtime-…

aa760e5

…lookups-later

Test

20e79dc

Enable JitDasmWithAlignmentBoundaries in Release

22f34a1

Merge branch 'main' of github.com:dotnet/runtime into expand-runtime-…

c3681c1

…lookups-later

EgorBo added 5 commits February 25, 2023 19:05

clean up

2110ec0

Revert "Enable JitDasmWithAlignmentBoundaries in Release"

54b224c

This reverts commit 22f34a1.

add hashtable

e4d4944

test

3e272e0

Initial version (no dynamic expansion for now)

2671e14

Add goto SCAN_BLOCK_AGAIN

a116695

jakobbotsch reviewed Mar 14, 2023

View reviewed changes

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

EgorBo and others added 3 commits March 14, 2023 15:27

Update src/coreclr/jit/runtimelookup.cpp

386446e

Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>

Address feedback

d2f3fec

update side effects for fallbackBb

19034d9

jakobbotsch approved these changes Mar 14, 2023

View reviewed changes

AndyAyersMS approved these changes Mar 14, 2023

View reviewed changes

EgorBo merged commit d86af84 into dotnet:main Mar 14, 2023

EgorBo deleted the expand-runtime-lookups-later branch March 14, 2023 22:33

EgorBo mentioned this pull request Mar 15, 2023

Runtime lookup clean up, enable for helper-based tail calls #83430

Merged

AndyAyersMS mentioned this pull request Mar 21, 2023

Regressions in System.Collections.CreateAddAndRemove #83738

Closed

kunalspathak mentioned this pull request Mar 21, 2023

[Perf] Linux/x64: 4 Improvements on 3/17/2023 5:15:23 PM dotnet/perf-autofiling-issues#14214

Closed

AndyAyersMS mentioned this pull request Mar 21, 2023

[Perf] Linux/x64: 18 Regressions on 3/15/2023 12:03:45 AM dotnet/perf-autofiling-issues#14193

Closed

EgorBo mentioned this pull request Mar 25, 2023

NativeAOT: Partially expand static initialization #83911

Merged

kunalspathak mentioned this pull request Mar 28, 2023

Consider expanding casts in late phases #84025

Open

AndyAyersMS mentioned this pull request Mar 30, 2023

JIT: better optimization for CORINFO_HELP_RUNTIMEHANDLE_METHOD #7590

Closed

JulieLeeMSFT mentioned this pull request Apr 4, 2023

What's new in .NET 8 Preview 3 dotnet/core#8135

Open

3 tasks

AndyAyersMS mentioned this pull request Apr 13, 2023

Regression in System.Collections.Concurrent.IsEmpty.Queue #84790

Closed

ghost locked as resolved and limited conversation to collaborators Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Expand runtime lookups in a late phase #81635

JIT: Expand runtime lookups in a late phase #81635

EgorBo commented Feb 4, 2023 •

edited

Loading

ghost commented Feb 4, 2023

This comment was marked as outdated.

AndyAyersMS commented Feb 6, 2023

jakobbotsch commented Feb 6, 2023

AndyAyersMS commented Feb 6, 2023

AndyAyersMS commented Feb 6, 2023

jkotas commented Feb 6, 2023 •

edited

Loading

EgorBo commented Feb 25, 2023

EgorBo commented Mar 14, 2023 •

edited

Loading

jakobbotsch left a comment

AndyAyersMS left a comment

AndyAyersMS Mar 14, 2023

EgorBo Mar 14, 2023

jakobbotsch Mar 14, 2023

EgorBo Mar 14, 2023

EgorBo Mar 14, 2023

AndyAyersMS Mar 14, 2023

EgorBo Mar 15, 2023 •

edited

Loading

AndyAyersMS Mar 14, 2023

EgorBo Mar 14, 2023

jkotas Mar 15, 2023

EgorBo Mar 15, 2023

EgorBo commented Mar 21, 2023 •

edited

Loading


		assert((lastIndOfTree != nullptr) && (pRuntimeLookup->indirections > 0));
		impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("bubbling QMark0"));

JIT: Expand runtime lookups in a late phase #81635

JIT: Expand runtime lookups in a late phase #81635

Conversation

EgorBo commented Feb 4, 2023 • edited Loading

ghost commented Feb 4, 2023

This comment was marked as outdated.

AndyAyersMS commented Feb 6, 2023

jakobbotsch commented Feb 6, 2023

AndyAyersMS commented Feb 6, 2023

AndyAyersMS commented Feb 6, 2023

jkotas commented Feb 6, 2023 • edited Loading

EgorBo commented Feb 25, 2023

EgorBo commented Mar 14, 2023 • edited Loading

jakobbotsch left a comment

Choose a reason for hiding this comment

AndyAyersMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorBo Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorBo commented Mar 21, 2023 • edited Loading

EgorBo commented Feb 4, 2023 •

edited

Loading

jkotas commented Feb 6, 2023 •

edited

Loading

EgorBo commented Mar 14, 2023 •

edited

Loading

EgorBo Mar 15, 2023 •

edited

Loading

EgorBo commented Mar 21, 2023 •

edited

Loading