JIT: Enable inlining for late devirtualization #110827

hez2010 · 2024-12-18T23:33:17Z

If we see a new inline candidate after late devirtualization, we can try marking and inlining it.
This unblocks the inlining and stack-allocating arbitrary ref-class enumerators (unless the inliner considers GetEnumerator as non-profitable). We might need to tune the inliner heuristics later to get more profitable inlining opportunities.

Contributes to #7541 and #108913

There're some really nice diffs: https://gist.github.com/MihuBot/29f7c64533ac1f38494fbfab361ab505

Example:

[MethodImpl(MethodImplOptions.NoInlining)]
static float GetDistance()
{
    IVector p1 = Vector.GetVector(4, 2);
    IVector p2 = Vector.GetVector(1, 6);
    IVector dir = p2.Sub(p1);
    return MathF.Sqrt(dir.X * dir.X + dir.Y * dir.Y);
}

struct Vector(float x, float y) : IVector
{
    public float X { get; set; } = x;
    public float Y { get; set; } = y;
    public readonly IVector Sub(IVector other) => GetVector(X - other.X, Y - other.Y);
    public static IVector GetVector(float x, float y) => new Vector(x, y);
}

interface IVector
{
    IVector Sub(IVector other);
    float X { get; set; }
    float Y { get; set; }
}

Before:

; Assembly listing for method Program:GetDistance():int (FullOpts)
G_M12138_IG01:  ;; offset=0x0000
       push     rbx
       sub      rsp, 48
                                                ;; size=5 bbWeight=1 PerfScore 1.25
G_M12138_IG02:  ;; offset=0x0005
       mov      rcx, 0x7FFD37F391F8      ; Vector
       call     CORINFO_HELP_NEWSFAST
       mov      rdx, rax
       mov      rcx, 0x4000000040800000
       mov      qword ptr [rdx+0x08], rcx
       mov      dword ptr [rsp+0x20], 0x3F800000
       mov      dword ptr [rsp+0x24], 0x40C00000
       lea      rcx, [rsp+0x20]
       call     [Vector:Sub(IVector):IVector:this]
       mov      rbx, rax
       mov      rcx, rbx
       mov      r11, 0x7FFD36EE0370      ; code for IVector:get_X():float:this
       call     [r11]IVector:get_X():float:this
       vmovss   dword ptr [rsp+0x2C], xmm0
       mov      rcx, rbx
       mov      r11, 0x7FFD36EE0378      ; code for IVector:get_X():float:this
       call     [r11]IVector:get_X():float:this
       vmulss   xmm0, xmm0, dword ptr [rsp+0x2C]
       vmovss   dword ptr [rsp+0x2C], xmm0
       mov      rcx, rbx
       mov      r11, 0x7FFD36EE0380      ; code for IVector:get_Y():float:this
       call     [r11]IVector:get_Y():float:this
       vmovss   dword ptr [rsp+0x28], xmm0
       mov      rcx, rbx
       mov      r11, 0x7FFD36EE0388      ; code for IVector:get_Y():float:this
       call     [r11]IVector:get_Y():float:this
       vmulss   xmm0, xmm0, dword ptr [rsp+0x28]
       vaddss   xmm0, xmm0, dword ptr [rsp+0x2C]
       vsqrtss  xmm0, xmm0, xmm0
                                                ;; size=166 bbWeight=1 PerfScore 51.50
G_M12138_IG03:  ;; offset=0x00AB
       add      rsp, 48
       pop      rbx
       ret
                                                ;; size=6 bbWeight=1 PerfScore 1.75

GDV:

; Assembly listing for method Program:GetDistance():int (Tier1)
G_M12138_IG01:  ;; offset=0x0000
       push     rsi
       push     rbx
       sub      rsp, 40
                                                ;; size=6 bbWeight=1 PerfScore 2.25
G_M12138_IG02:  ;; offset=0x0006
       mov      rbx, 0x7FFD4531D278      ; Vector
       mov      rcx, rbx
       call     CORINFO_HELP_NEWSFAST
       mov      rsi, rax
       mov      rcx, 0x4000000040800000
       mov      qword ptr [rsi+0x08], rcx
       mov      rcx, rbx
       call     CORINFO_HELP_NEWSFAST
       mov      rcx, 0x40C000003F800000
       mov      qword ptr [rax+0x08], rcx
       add      rax, 8
       vmovss   xmm0, dword ptr [rax]
       vsubss   xmm0, xmm0, dword ptr [rsi+0x08]
       vmovss   xmm1, dword ptr [rax+0x04]
       vsubss   xmm1, xmm1, dword ptr [rsi+0x0C]
       vmovss   dword ptr [rsp+0x24], xmm0
       vmovss   dword ptr [rsp+0x20], xmm1
       mov      rcx, rbx
       call     CORINFO_HELP_NEWSFAST
       vmovss   xmm0, dword ptr [rsp+0x24]
       vmovss   dword ptr [rax+0x08], xmm0
       vmovss   xmm1, dword ptr [rsp+0x20]
       vmovss   dword ptr [rax+0x0C], xmm1
       vmovss   xmm0, dword ptr [rax+0x08]
       vmovaps  xmm1, xmm0
       vmulss   xmm0, xmm1, xmm0
       vmovss   xmm1, dword ptr [rax+0x0C]
       vmovaps  xmm2, xmm1
       vmulss   xmm1, xmm2, xmm1
       vaddss   xmm0, xmm1, xmm0
       vsqrtss  xmm0, xmm0, xmm0
                                                ;; size=156 bbWeight=1 PerfScore 67.50
G_M12138_IG03:  ;; offset=0x00A2
       add      rsp, 40
       pop      rbx
       pop      rsi
       ret
                                                ;; size=7 bbWeight=1 PerfScore 2.25

After:

; Assembly listing for method Program:GetDistance():int (FullOpts)
G_M12138_IG01:  ;; offset=0x0000
                                                ;; size=0 bbWeight=1 PerfScore 0.00
G_M12138_IG02:  ;; offset=0x0000
       vmovss   xmm0, dword ptr [reloc @RWD00]
                                                ;; size=8 bbWeight=1 PerfScore 3.00
G_M12138_IG03:  ;; offset=0x0008
       ret
                                                ;; size=1 bbWeight=1 PerfScore 1.00
RWD00   dd      40A00000h               ;         5

/cc: @AndyAyersMS

hez2010 · 2025-01-20T21:21:28Z

@MihuBot

hez2010 · 2025-01-20T22:56:09Z

@MihuBot

hez2010 · 2025-01-20T23:20:24Z

cc @AndyAyersMS
By enabling this we managed to inline all kinds of ref-class enumerators and stack allocated them: https://gist.github.com/MihuBot/0a4cb4714afc1ff5fe474f99df1ce6dd

AndyAyersMS · 2025-01-21T15:41:03Z

Skimmed the changes and it seems promising. I'll take a deeper look soon.

src/coreclr/jit/gentree.cpp

AndyAyersMS

I think this is in good shape, just one last small thing.

AndyAyersMS

This LGTM -- @jakobbotsch any other feedback?

hez2010 · 2025-01-28T14:34:42Z

I'm testing TP impact of splitting trees in #111896. If it turns out that the TP impact has the similar pattern with the check in this PR, I will merge it into this branch.

jakobbotsch

LGTM as well as is

hez2010 · 2025-01-28T16:18:23Z

Seems that TP with splitting the tree doesn't have meaningful change: https://dev.azure.com/dnceng-public/public/_build/results?buildId=931974&view=ms.vss-build-web.run-extensions-tab

Merged the commits into this PR.

jakobbotsch · 2025-01-28T16:31:23Z

src/coreclr/jit/fginline.cpp

+                {
+                    Statement* newStmt = nullptr;
+                    GenTree**  callUse = nullptr;
+                    if (m_compiler->gtSplitTree(m_compiler->compCurBB, m_curStmt, call, &newStmt, &callUse))


gtSplitTree needs to learn that it has to split lvHasLdAddrOp locals when it runs early.

AndyAyersMS · 2025-01-28T16:57:45Z

src/coreclr/jit/fginline.cpp

+                {
+                    Statement* newStmt = nullptr;
+                    GenTree**  callUse = nullptr;
+                    if (m_compiler->gtSplitTree(m_compiler->compCurBB, m_curStmt, call, &newStmt, &callUse, true))


Are you using context != nullptr as a check if devirtualization succeeded? A better check is !call->IsVirtual().

We should avoid splitting the tree if devirtualization fails, and also if the call can't be made into an inline candidate (seems like impMarkInlineCandidate can be called on a non-root call...?)

So maybe reorder things a bit.

I'd also be fine if you revert all this and go back to the non-split version.

Also it's not obvious that splitting a tree that we're walking will work out as expected. I think it's ok here, especially since we back up and will reprocess the current tree, but it feels like it would be cleaner if we aborted the walk at this point.

Yeah context will be nullptr if devirtualization failed.
Switched to use !call.IsVirtual() instead. The context will be used while calling impMarkInlineCandidate so I added an assertion to make sure it's correctly set.

Resolved.

AndyAyersMS

Looking good. Will give @jakobbotsch one last chance to look too.

jakobbotsch · 2025-01-28T23:07:46Z

src/coreclr/jit/fginline.cpp

+                    {
+                        Statement* newStmt = nullptr;
+                        GenTree**  callUse = nullptr;
+                        if (m_compiler->gtSplitTree(m_compiler->compCurBB, m_curStmt, call, &newStmt, &callUse, true))


Strictly speaking this split is unnecessarily conservative: it will introduce temps for the arguments of the call, which can bypass optimizations the inliner can make depending on the specific shape of the argument. Fixing that requires enhancing gtSplitTree a bit (see e.g. my version in this branch), however, even with that done you will run into issues around incorrect treatment of the retbuffer in the inliner. So I wouldn't suggest trying to do anything about it at this point, but it is a potential future improvement.

jakobbotsch

LGTM

AndyAyersMS · 2025-01-29T00:03:09Z

@hez2010 thanks again!

* main: Make cdac APIs public but experimental (dotnet#111180) JIT: Enable inlining for late devirtualization (dotnet#110827) Remove unsafe `bool` casts (dotnet#111024) Fix shimmed implementation of TryGetHashAndReset to handle HMAC.

amanasifkhalid · 2025-02-10T20:37:12Z

src/coreclr/jit/fginline.cpp

+                        }
+
+                        call->GetSingleInlineCandidateInfo()->exactContextHandle = context;
+                        INDEBUG(call->GetSingleInlineCandidateInfo()->inlinersContext = call->gtInlineContext);


@hez2010 I think this Debug logic is causing Debug and Release JITs to report differing amounts of metadata to SuperPMI. runtime-coreclr superpmi-asmdiffs-checked-release broke around the time this PR was merged, and I'm not getting any diffs locally when using builds from the commit before this one.

cc @AndyAyersMS @jakobbotsch. I'm not sure what the ideal fix is here, since GenTreeCall::gtInlineContext is Debug-only, and removing this logic altogether triggers Debug asserts in InlineStrategy::NewContext.

I think we may need to move call->gtInlineContext out of #if DEBUG and always set call->GetSingleInlineCandidateInfo()->inlinersContext accordingly, otherwise inlinersContext can sometimes be incorrect because we don't keep tracking it after the importer is done, and leading to inlining failure (which will be caught silently and reverted so it won't affect the correctness of codegen). This may explain why you are seeing different amounts of metadata to SuperPMI, where in Release we inline fewer methods due to the incorrect inlinersContext.

Can the information be put into LateDevirtualizationInfo?

We have avoided trying to add more fields to GenTreeCall that are only needed for some small subset of all calls.

What @jakobbotsch suggests makes sense to me.

Okay I revised this issue and found that the issue is that the debug logic should really be

INDEBUG(call->gtInlineContext = call->GetSingleInlineCandidateInfo()->inlinersContext);

instead.

We set a new inlinersContext for the inline candidate, but didn't update the original InlineContext for the call. Sorry for this dumb typo mistake... I should really have noticed this before.

The fix is now live: #112396

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

hez2010 changed the title ~~Enable inlining for late devirtualization~~ JIT: Enable inlining for late devirtualization Dec 18, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 18, 2024

hez2010 force-pushed the inline-exact-devirt branch 2 times, most recently from 6173a81 to e287f69 Compare January 20, 2025 18:40

Enable inlining for late devirt

686f64e

hez2010 force-pushed the inline-exact-devirt branch from e287f69 to 686f64e Compare January 20, 2025 18:41

MihuBot mentioned this pull request Jan 20, 2025

[JitDiff X64] [hez2010] JIT: Enable inlining for late devirtualization MihuBot/runtime-utils#912

Open

hez2010 added 2 commits January 21, 2025 04:14

Pass correct IL offset

27cc1ce

Only creates RET_EXPR if the node is not top-level or not returning void

21a5adb

MihuBot mentioned this pull request Jan 20, 2025

[JitDiff X64] [hez2010] JIT: Enable inlining for late devirtualization MihuBot/runtime-utils#913

Open

Do not try inlining if BBF_INTERNAL is set

d1d5181

MihuBot mentioned this pull request Jan 20, 2025

[JitDiff X64] [hez2010] JIT: Enable inlining for late devirtualization MihuBot/runtime-utils#914

Open

hez2010 added 2 commits January 21, 2025 07:00

Ensure the type matches

cd9f817

Set inliner context correctly

aad3d7e

MihuBot mentioned this pull request Jan 20, 2025

[JitDiff X64] [hez2010] JIT: Enable inlining for late devirtualization MihuBot/runtime-utils#915

Open

Merge branch 'main' into inline-exact-devirt

3f5204b

hez2010 marked this pull request as ready for review January 21, 2025 10:48

AndyAyersMS reviewed Jan 27, 2025

View reviewed changes

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

AndyAyersMS reviewed Jan 27, 2025

View reviewed changes

hez2010 added 2 commits January 28, 2025 02:21

Make lvHasLdAddrOp check optional

0457417

Rename to early

b27a25a

AndyAyersMS approved these changes Jan 27, 2025

View reviewed changes

hez2010 added 2 commits January 28, 2025 20:40

Split effects if necessary

6c0e11b

Use gtSplitTree

5de54f8

jakobbotsch approved these changes Jan 28, 2025

View reviewed changes

jakobbotsch reviewed Jan 28, 2025

View reviewed changes

Teach gtSplitTree to support early use

1e967c8

AndyAyersMS reviewed Jan 28, 2025

View reviewed changes

hez2010 added 3 commits January 29, 2025 02:08

Cleanup

10cd910

ClearInlineInfo is not needed

067064f

Merge branch 'main' into inline-exact-devirt

6e067ce

AndyAyersMS approved these changes Jan 28, 2025

View reviewed changes

jakobbotsch reviewed Jan 28, 2025

View reviewed changes

jakobbotsch approved these changes Jan 28, 2025

View reviewed changes

AndyAyersMS merged commit a9d67ec into dotnet:main Jan 29, 2025
112 checks passed

This was referenced Jan 29, 2025

JIT: Update type when return temp is freshly created #111948

Merged

JIT: devirtualization next steps #7541

Open

JIT: De-abstraction in .NET 10 #108913

Open

LoopedBard3 mentioned this pull request Feb 4, 2025

[Perf] Windows/x64: 2 FrozenDictionary Regressions on 1/29/2025 3:52:02 AM +00:00 #112159

Open

hez2010 mentioned this pull request Feb 7, 2025

Devirtualization isn't acting on devirtualizable calls past the first #39519

Closed

amanasifkhalid reviewed Feb 10, 2025

View reviewed changes

hez2010 mentioned this pull request Feb 11, 2025

JIT: Always track the context for late devirt #112396

Merged

jakobbotsch mentioned this pull request Feb 11, 2025

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Enable inlining for late devirtualization #110827

JIT: Enable inlining for late devirtualization #110827

hez2010 commented Dec 18, 2024 •

edited

Loading

hez2010 commented Jan 20, 2025

hez2010 commented Jan 20, 2025

hez2010 commented Jan 20, 2025

AndyAyersMS commented Jan 21, 2025

AndyAyersMS left a comment

AndyAyersMS left a comment

hez2010 commented Jan 28, 2025

jakobbotsch left a comment

hez2010 commented Jan 28, 2025 •

edited

Loading

jakobbotsch Jan 28, 2025

hez2010 Jan 28, 2025

AndyAyersMS Jan 28, 2025

AndyAyersMS Jan 28, 2025

hez2010 Jan 28, 2025 •

edited

Loading

AndyAyersMS left a comment

jakobbotsch Jan 28, 2025

jakobbotsch left a comment

AndyAyersMS commented Jan 29, 2025

amanasifkhalid Feb 10, 2025

hez2010 Feb 10, 2025 •

edited

Loading

jakobbotsch Feb 10, 2025

AndyAyersMS Feb 10, 2025

hez2010 Feb 11, 2025 •

edited

Loading

JIT: Enable inlining for late devirtualization #110827

JIT: Enable inlining for late devirtualization #110827

Conversation

hez2010 commented Dec 18, 2024 • edited Loading

hez2010 commented Jan 20, 2025

hez2010 commented Jan 20, 2025

hez2010 commented Jan 20, 2025

AndyAyersMS commented Jan 21, 2025

AndyAyersMS left a comment

Choose a reason for hiding this comment

AndyAyersMS left a comment

Choose a reason for hiding this comment

hez2010 commented Jan 28, 2025

jakobbotsch left a comment

Choose a reason for hiding this comment

hez2010 commented Jan 28, 2025 • 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

hez2010 Jan 28, 2025 • edited Loading

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

jakobbotsch left a comment

Choose a reason for hiding this comment

AndyAyersMS commented Jan 29, 2025

Choose a reason for hiding this comment

hez2010 Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hez2010 Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

hez2010 commented Dec 18, 2024 •

edited

Loading

hez2010 commented Jan 28, 2025 •

edited

Loading

hez2010 Jan 28, 2025 •

edited

Loading

hez2010 Feb 10, 2025 •

edited

Loading

hez2010 Feb 11, 2025 •

edited

Loading