Handle side-effects in impBoxPatternMatch #90496

EgorBo · 2023-08-13T19:29:01Z

Closes #90492
Contributes to #9120

The main motivation is to get rid of [MethodImpl(MethodImplOptions.AggressiveOptimization)] from AsyncTaskMethodBuilderT.cs it was placed there to avoid unnecessary allocation Tier0 used to produce while Tier-FullOpts did not. New tier0 codegen for that method:

; Assembly listing for method System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1[System.Threading.Tasks.VoidTaskResult]:AwaitUnsafeOnCompleted[System.Runtime.CompilerServices.TaskAwaiter](byref,System.Runtime.CompilerServices.IAsyncStateMachineBox) (Tier0)

G_M46438_IG01:  ;; offset=0x0000
       push     rbp
       sub      rsp, 80
       lea      rbp, [rsp+0x50]
       vxorps   xmm4, xmm4, xmm4
       vmovdqa  xmmword ptr [rbp-0x30], xmm4
       vmovdqa  xmmword ptr [rbp-0x20], xmm4
       vmovdqa  xmmword ptr [rbp-0x10], xmm4
       mov      bword ptr [rbp+0x10], rcx
       mov      gword ptr [rbp+0x18], rdx
						;; size=37 bbWeight=1 PerfScore 10.08
G_M46438_IG02:  ;; offset=0x0025
       xor      ecx, ecx
       mov      gword ptr [rbp-0x08], rcx
       mov      rcx, bword ptr [rbp+0x10]
       mov      rcx, gword ptr [rcx]
       mov      gword ptr [rbp-0x30], rcx
       mov      rcx, bword ptr [rbp+0x10]
       mov      bword ptr [rbp-0x10], rcx
       mov      rcx, bword ptr [rbp-0x10]
       mov      rcx, gword ptr [rcx]
       mov      rdx, gword ptr [rbp+0x18]
       mov      r8d, 1
       call     [System.Runtime.CompilerServices.TaskAwaiter:UnsafeOnCompletedInternal(System.Threading.Tasks.Task,System.Runtime.CompilerServices.IAsyncStateMachineBox,bool)]
       nop      
						;; size=49 bbWeight=1 PerfScore 14.75
G_M46438_IG03:  ;; offset=0x0056
       add      rsp, 80
       pop      rbp
       ret      
						;; size=6 bbWeight=1 PerfScore 1.75

; Total bytes of code 92

ghost · 2023-08-13T19:29:10Z

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

Issue Details

Closes #90492

The changes are not big if you ignore white-spaces in code-review tab

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

stephentoub · 2023-08-13T19:40:37Z

can we also delete

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs

Line 96 in f6d2220

    
           [MethodImpl(MethodImplOptions.AggressiveOptimization)] // workaround boxing allocations in Tier0: https://github.com/dotnet/runtime/issues/9120

?

EgorBo · 2023-08-13T20:02:56Z

can we also delete

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs

Line 96 in f6d2220

[MethodImpl(MethodImplOptions.AggressiveOptimization)] // workaround boxing allocations in Tier0: https://github.com/dotnet/runtime/issues/9120

?

~~so there is still one allocation I can't yet eliminate~~

Looks like it can be removed, Tier0 codegen:

; Assembly listing for method System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1[System.Threading.Tasks.VoidTaskResult]:AwaitUnsafeOnCompleted[System.Runtime.CompilerServices.TaskAwaiter](byref,System.Runtime.CompilerServices.IAsyncStateMachineBox) (Tier0)

G_M46438_IG01:  ;; offset=0x0000
       push     rbp
       sub      rsp, 80
       lea      rbp, [rsp+0x50]
       vxorps   xmm4, xmm4, xmm4
       vmovdqa  xmmword ptr [rbp-0x30], xmm4
       vmovdqa  xmmword ptr [rbp-0x20], xmm4
       vmovdqa  xmmword ptr [rbp-0x10], xmm4
       mov      bword ptr [rbp+0x10], rcx
       mov      gword ptr [rbp+0x18], rdx
						;; size=37 bbWeight=1 PerfScore 10.08
G_M46438_IG02:  ;; offset=0x0025
       xor      ecx, ecx
       mov      gword ptr [rbp-0x08], rcx
       mov      rcx, bword ptr [rbp+0x10]
       mov      rcx, gword ptr [rcx]
       mov      gword ptr [rbp-0x30], rcx
       mov      rcx, bword ptr [rbp+0x10]
       mov      bword ptr [rbp-0x10], rcx
       mov      rcx, bword ptr [rbp-0x10]
       mov      rcx, gword ptr [rcx]
       mov      rdx, gword ptr [rbp+0x18]
       mov      r8d, 1
       call     [System.Runtime.CompilerServices.TaskAwaiter:UnsafeOnCompletedInternal(System.Threading.Tasks.Task,System.Runtime.CompilerServices.IAsyncStateMachineBox,bool)]
       nop      
						;; size=49 bbWeight=1 PerfScore 14.75
G_M46438_IG03:  ;; offset=0x0056
       add      rsp, 80
       pop      rbp
       ret      
						;; size=6 bbWeight=1 PerfScore 1.75

; Total bytes of code 92

Previous Tier0 codegen: https://gist.github.com/EgorBo/7da915602c4d826e0e7d0d981aafc08d

EgorBo · 2023-08-14T01:04:05Z

@MihuBot --tier0

EgorBo · 2023-08-14T02:12:59Z

@MihuBot

EgorBo · 2023-08-14T07:14:43Z

@jakobbotsch @dotnet/jit-contrib PTAL, I recommend disabling "whitespaces" for review.

impBoxPatternMatch used to be conservative about side-effects, now I spill them as separate statements.
Diffs aren't big but at least I was able to remove AggressiveOpt.

EgorBo · 2023-08-14T07:17:07Z

src/coreclr/jit/importer.cpp

-                                    impResolveToken(codeAddr + 1, &isInstResolvedToken, CORINFO_TOKENKIND_Casting);
+                            }
+                            else if ((foldAsHelper == CORINFO_HELP_BOX_NULLABLE) &&
+                                     ((impStackTop().val->gtFlags & GTF_SIDE_EFFECT) == 0))


Didn't do it for nullable due to empty diffs for it (would have to add tests as the logic is a bit different).

jakobbotsch · 2023-08-14T09:12:42Z

src/coreclr/jit/importer.cpp

+                    GenTree* op = impPopStack().val;
+                    if ((op->gtFlags & GTF_SIDE_EFFECT) != 0)
                    {
-                        JITDUMP("\n Importing BOX; BR_TRUE/FALSE as %sconstant\n",
-                                treeToNullcheck == nullptr ? "" : "nullcheck+");
-                        impPopStack();
-
-                        GenTree* result = gtNewIconNode(1);
-
-                        if (treeToNullcheck != nullptr)
-                        {
-                            GenTree* nullcheck = gtNewNullCheck(treeToNullcheck, compCurBB);
-                            result             = gtNewOperNode(GT_COMMA, TYP_INT, nullcheck, result);
-                        }
-
-                        impPushOnStack(result, typeInfo(TYP_INT));
-                        return 0;
+                        impStoreTemp(lvaGrabTemp(true DEBUGARG("spill side effects")), op, CHECK_SPILL_ALL);
                    }


This is a bit of an unusual way of doing this in the importer, use impSpillSideEffect instead?

Also we already do this from the caller for BoxPatterns::IsByRefLike. Should it be unified?

Ah, thanks for the hint. addressed.

Also we already do this from the caller for BoxPatterns::IsByRefLike. Should it be unified?

Looks like that one has to be always done regardless wether we match a pattern or not while here we only do this when we match it. Presumably the byref case is rare so it's ok to call it twice for simplicity?

Looks like that one has to be always done regardless wether we match a pattern or not while here we only do this when we match it.

I don't think so (note that the comment is misleading -- we BADCODE today on unmatched patterns, we do not generate code to throw InvalidProgramException at runtime). But I'm ok with leaving it to avoid further risk and potentially cleaning it up in .NET 9, assuming you want this in .NET 8.

Ah, I see, yeah let's clean it up in .NET 9.0, it has a potential to slightly refactor the whole thing

jakobbotsch · 2023-08-14T11:25:30Z

src/coreclr/jit/importer.cpp

-                        impPushOnStack(result, typeInfo(TYP_INT));
-                        return 0;
-                    }
+                    impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("spilling side-effects"));


Did you check diffs for this? I'm not sure we want to spill all side effects, it should be enough to spill the top one (and the slots it interferes with). I also don't think we need to pass true. Not sure if there is any significant difference given the side-effecting trees we would expect at the top of the stack here, though.

Diffs - I've also checked that all side-effects are preserved in my tests

Be less conservative in impBoxPatternMatch

9613a71

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

ghost assigned EgorBo Aug 13, 2023

build-analysis bot mentioned this pull request Aug 13, 2023

Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488

Open

EgorBo added 4 commits August 14, 2023 02:16

Fix asserts

f791c02

Actually, it's no longer needed

168e6ec

clean up

587eb27

Update importer.cpp

9eefa4e

MihuBot mentioned this pull request Aug 14, 2023

[X64] [EgorBo] Handle side-effects in impBoxPatternMatch MihuBot/runtime-utils#163

Open

MihuBot mentioned this pull request Aug 14, 2023

[X64] [EgorBo] Handle side-effects in impBoxPatternMatch MihuBot/runtime-utils#164

Open

EgorBo marked this pull request as ready for review August 14, 2023 03:27

MihuBot mentioned this pull request Aug 14, 2023

[X64] [EgorBo] Handle side-effects in impBoxPatternMatch MihuBot/runtime-utils#165

Open

EgorBo commented Aug 14, 2023

View reviewed changes

jakobbotsch reviewed Aug 14, 2023

View reviewed changes

EgorBo added 2 commits August 14, 2023 13:17

Address feedback

9335402

wrong one

313cef2

jakobbotsch reviewed Aug 14, 2023

View reviewed changes

jakobbotsch approved these changes Aug 14, 2023

View reviewed changes

less conservative

4372334

EgorBo merged commit 8323e58 into dotnet:main Aug 14, 2023

EgorBo deleted the handle-sideeffects-in-impBoxPatternMatch branch August 14, 2023 14:50

sebastienros mentioned this pull request Aug 21, 2023

Performance regression, memory usage #90899

Closed

jakobbotsch mentioned this pull request Aug 22, 2023

Regressions in System.Collections.Sort #90933

Closed

EgorBo mentioned this pull request Aug 23, 2023

[release/8.0] Bring AggressiveOptimization back on AwaitUnsafeOnCompleted #90980

Merged

ghost locked as resolved and limited conversation to collaborators Sep 15, 2023

Handle side-effects in impBoxPatternMatch #90496

Handle side-effects in impBoxPatternMatch #90496

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ghost commented Aug 13, 2023

Uh oh!

stephentoub commented Aug 13, 2023

Uh oh!

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

Uh oh!

Uh oh!

EgorBo commented Aug 14, 2023

Uh oh!

EgorBo commented Aug 14, 2023

Uh oh!

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

Uh oh!

Uh oh!

EgorBo Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

EgorBo Aug 14, 2023 • 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!

jakobbotsch Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

EgorBo Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

EgorBo Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EgorBo commented Aug 13, 2023 •

edited

Loading

EgorBo commented Aug 13, 2023 •

edited

Loading

EgorBo commented Aug 14, 2023 •

edited

Loading

EgorBo Aug 14, 2023 •

edited

Loading