JIT: Enable inlining for late devirtualization by hez2010 · Pull Request #110827 · dotnet/runtime

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)]staticfloatGetDistance(){IVectorp1=Vector.GetVector(4,2);IVectorp2=Vector.GetVector(1,6);IVectordir=p2.Sub(p1);returnMathF.Sqrt(dir.X*dir.X+dir.Y*dir.Y);}structVector(floatx,floaty):IVector{publicfloatX{get;set;}=x;publicfloatY{get;set;}=y;publicreadonlyIVectorSub(IVectorother)=>GetVector(X-other.X,Y-other.Y);publicstaticIVectorGetVector(floatx,floaty)=>newVector(x,y);}interfaceIVector{IVectorSub(IVectorother);floatX{get;set;}floatY{get;set;}}

Before:

; Assembly listing for method Program:GetDistance():int (FullOpts)G_M12138_IG01: ;; offset=0x0000pushrbxsubrsp,48 ;; size=5 bbWeight=1 PerfScore 1.25G_M12138_IG02: ;; offset=0x0005movrcx,0x7FFD37F391F8 ; Vectorcall CORINFO_HELP_NEWSFASTmovrdx,raxmovrcx,0x4000000040800000mov qword ptr [rdx+0x08],rcxmov dword ptr [rsp+0x20],0x3F800000mov dword ptr [rsp+0x24],0x40C00000learcx,[rsp+0x20]call[Vector:Sub(IVector):IVector:this]movrbx,raxmovrcx,rbxmovr11,0x7FFD36EE0370 ; code for IVector:get_X():float:thiscall[r11]IVector:get_X():float:thisvmovss dword ptr [rsp+0x2C],xmm0movrcx,rbxmovr11,0x7FFD36EE0378 ; code for IVector:get_X():float:thiscall[r11]IVector:get_X():float:this vmulss xmm0,xmm0, dword ptr [rsp+0x2C]vmovss dword ptr [rsp+0x2C],xmm0movrcx,rbxmovr11,0x7FFD36EE0380 ; code for IVector:get_Y():float:thiscall[r11]IVector:get_Y():float:thisvmovss dword ptr [rsp+0x28],xmm0movrcx,rbxmovr11,0x7FFD36EE0388 ; code for IVector:get_Y():float:thiscall[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.50G_M12138_IG03: ;; offset=0x00ABaddrsp,48poprbxret ;; size=6 bbWeight=1 PerfScore 1.75

GDV:

; Assembly listing for method Program:GetDistance():int (Tier1)G_M12138_IG01: ;; offset=0x0000pushrsipushrbxsubrsp,40 ;; size=6 bbWeight=1 PerfScore 2.25G_M12138_IG02: ;; offset=0x0006movrbx,0x7FFD4531D278 ; Vectormovrcx,rbxcall CORINFO_HELP_NEWSFASTmovrsi,raxmovrcx,0x4000000040800000mov qword ptr [rsi+0x08],rcxmovrcx,rbxcall CORINFO_HELP_NEWSFASTmovrcx,0x40C000003F800000mov qword ptr [rax+0x08],rcxaddrax,8vmovssxmm0, dword ptr [rax] vsubss xmm0,xmm0, dword ptr [rsi+0x08]vmovssxmm1, dword ptr [rax+0x04] vsubss xmm1,xmm1, dword ptr [rsi+0x0C]vmovss dword ptr [rsp+0x24],xmm0vmovss dword ptr [rsp+0x20],xmm1movrcx,rbxcall CORINFO_HELP_NEWSFASTvmovssxmm0, dword ptr [rsp+0x24]vmovss dword ptr [rax+0x08],xmm0vmovssxmm1, dword ptr [rsp+0x20]vmovss dword ptr [rax+0x0C],xmm1vmovssxmm0, dword ptr [rax+0x08] vmovaps xmm1,xmm0 vmulss xmm0,xmm1,xmm0vmovssxmm1, 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.50G_M12138_IG03: ;; offset=0x00A2addrsp,40poprbxpoprsiret ;; 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.00G_M12138_IG02: ;; offset=0x0000vmovssxmm0, dword ptr [reloc @RWD00] ;; size=8 bbWeight=1 PerfScore 3.00G_M12138_IG03: ;; offset=0x0008ret ;; size=1 bbWeight=1 PerfScore 1.00RWD00 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.

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

+ {
+ 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

+ {
+ 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

+ {
+ 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

+ }
+
+ 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

ghost 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 devirtualizationDec 18, 2024

dotnet-policy-serviceBot 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 e287f69Compare January 20, 2025 18:40

Enable inlining for late devirt
686f64e

hez2010 force-pushed the inline-exact-devirt branch from e287f69 to 686f64eCompare 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

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:mainJan 29, 2025

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
Closed

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
Closed

hez2010 mentioned this pull request Mar 4, 2025
Add .NET 10 Preview 2 release notes -- Libraries and Runtime dotnet/core#9770
Merged

github-actionsBot locked and limited conversation to collaborators Mar 13, 2025

Uh oh!

Conversation

hez2010 commented Dec 18, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hez2010 commented Jan 20, 2025

Uh oh!

hez2010 commented Jan 20, 2025

Uh oh!

hez2010 commented Jan 20, 2025

Uh oh!

AndyAyersMS commented Jan 21, 2025

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

hez2010 commented Jan 28, 2025

Uh oh!

jakobbotsch left a comment

Choose a reason for hiding this comment

Uh oh!

hez2010 commented Jan 28, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hez2010Jan 28, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch left a comment

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Jan 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hez2010Feb 10, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hez2010Feb 11, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hez2010 commented Dec 18, 2024•
edited
Loading

hez2010 commented Jan 28, 2025•
edited
Loading

hez2010Jan 28, 2025•
edited
Loading

hez2010Feb 10, 2025•
edited
Loading

hez2010Feb 11, 2025•
edited
Loading