Uh oh!
There was an error while loading. Please reload this page.
Handle side-effects in impBoxPatternMatch - #90496
Conversation
ghost
commented
Aug 13, 2023
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsCloses #90492 The changes are not big if you ignore white-spaces in code-review tab
|
stephentoub
commented
Aug 13, 2023
can we also delete ? |
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=0x0000pushrbpsubrsp,80learbp,[rsp+0x50] vxorps xmm4,xmm4,xmm4 vmovdqa xmmword ptr [rbp-0x30],xmm4 vmovdqa xmmword ptr [rbp-0x20],xmm4 vmovdqa xmmword ptr [rbp-0x10],xmm4mov bword ptr [rbp+0x10],rcxmov gword ptr [rbp+0x18],rdx ;; size=37 bbWeight=1 PerfScore 10.08G_M46438_IG02: ;; offset=0x0025xorecx,ecxmov gword ptr [rbp-0x08],rcxmovrcx, bword ptr [rbp+0x10]movrcx, gword ptr [rcx]mov gword ptr [rbp-0x30],rcxmovrcx, bword ptr [rbp+0x10]mov bword ptr [rbp-0x10],rcxmovrcx, bword ptr [rbp-0x10]movrcx, gword ptr [rcx]movrdx, gword ptr [rbp+0x18]movr8d,1call[System.Runtime.CompilerServices.TaskAwaiter:UnsafeOnCompletedInternal(System.Threading.Tasks.Task,System.Runtime.CompilerServices.IAsyncStateMachineBox,bool)]nop ;; size=49 bbWeight=1 PerfScore 14.75G_M46438_IG03: ;; offset=0x0056addrsp,80poprbpret ;; size=6 bbWeight=1 PerfScore 1.75; Total bytes of code 92Previous Tier0 codegen: https://gist.github.com/EgorBo/7da915602c4d826e0e7d0d981aafc08d |
EgorBo
commented
Aug 14, 2023
@MihuBot --tier0 |
EgorBo
commented
Aug 14, 2023
@jakobbotsch @dotnet/jit-contrib PTAL, I recommend disabling "whitespaces" for review.
|
| impResolveToken(codeAddr + 1, &isInstResolvedToken, CORINFO_TOKENKIND_Casting); | ||
| } | ||
| else if ((foldAsHelper == CORINFO_HELP_BOX_NULLABLE) && | ||
| ((impStackTop().val->gtFlags & GTF_SIDE_EFFECT) == 0)) |
There was a problem hiding this comment.
Didn't do it for nullable due to empty diffs for it (would have to add tests as the logic is a bit different).
| 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); | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah, I see, yeah let's clean it up in .NET 9.0, it has a potential to slightly refactor the whole thing
| impPushOnStack(result, typeInfo(TYP_INT)); | ||
| return 0; | ||
| } | ||
| impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("spilling side-effects")); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Diffs - I've also checked that all side-effects are preserved in my tests
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: