JIT: Rarely run exception block is not placed at the end, regression

[BoyBaykiller]

In the Get function the rarely run exception throwing block is not placed after the vector access causing a jmp on every call. This is a regression to .NET 9. In GetFixed I rewrote the if statement which fixes it.
https://godbolt.org/z/M4dvTv1ah

static float Get(Vector2 vec, int index)
{
    if (index > 1 || index < 0)
    {
        ThrowOutOfRangeException("You tried to access this vector at index: {0}", index);
    }

    return *(&vec.X + index);
}

static float GetFixed(Vector2 vec, int index)
{
    if ((uint)index > 1)
    {
        ThrowOutOfRangeException("You tried to access this vector at index: {0}", index);
    }

    return *(&vec.X + index);
}

Program:Get(Program+Vector2,int):float (FullOpts):
       push     rbx
       sub      rsp, 16
       vmovsd   qword ptr [rsp], xmm0
       vmovss   dword ptr [rsp+0x08], xmm1
       mov      ebx, edi
       cmp      ebx, 1
       jbe      SHORT G_M37698_IG04
       mov      edi, 1
       mov      rsi, 0x7F996EC7D118
       call     [CORINFO_HELP_STRCNS]
       mov      rdi, rax
       mov      esi, ebx
       call     [Program:ThrowOutOfRangeException[int](System.String,int)]
       int3     
G_M37698_IG04:  ;; offset=0x0038
       lea      rax, bword ptr [rsp]
       mov      ecx, ebx
       vmovss   xmm0, dword ptr [rax+4*rcx]
       add      rsp, 16
       pop      rbx
       ret      

Program:GetFixed(Program+Vector2,int):float (FullOpts):
       push     rbx
       sub      rsp, 16
       vmovsd   qword ptr [rsp], xmm0
       vmovss   dword ptr [rsp+0x08], xmm1
       mov      ebx, edi
       cmp      ebx, 1
       ja       SHORT G_M29684_IG04
       lea      rdi, bword ptr [rsp]
       mov      esi, ebx
       vmovss   xmm0, dword ptr [rdi+4*rsi]
       add      rsp, 16
       pop      rbx
       ret      
G_M29684_IG04:  ;; offset=0x0028
       mov      edi, 91
       mov      rsi, 0x7F996EC7D118
       call     [CORINFO_HELP_STRCNS]
       mov      rdi, rax
       mov      esi, ebx
       call     [Program:ThrowOutOfRangeException[int](System.String,int)]
       int3     

[dotnet-policy-service]

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

[EgorBo]

@amanasifkhalid could it be regressed by some layout work?

[amanasifkhalid]

I can reproduce this with tiering disabled. Based on the edge likelihoods we're coming up with, block layout is doing the "right" thing:

*************** In fgSearchImprovedLayout()

Initial BasicBlocks
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight   [IL range]   [jump]                            [EH region]        [flags]
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1    [000..004)-> BB04(0.25),BB03(0.75)   ( cond )                   i LIR
BB03 [0002]  1       BB01                  0.50 [008..013)                           (throw )                   i LIR hascall gcsafe
BB04 [0003]  1       BB01                  1    [013..022)                           (return)                   i LIR
---------------------------------------------------------------------------------------------------------------------------------------------------------------------

Compacting hot jumps
Initial layout cost: 175.000000
Running greedy 3-opt pass.
No changes made.
Reordering block list

*************** Finishing PHASE Optimize layout [no changes]

As for why we biased the edge likelihoods toward the throw block, in the absence of profile data, we frequently come up with 50/50 likelihood splits, and because the throw statement is hidden in ThrowOutOfRangeException, we don't know that BB03 throws until after inlining runs. So we initially come up with 50/50 splits for each condition in the if-statement (bool opts later collapses this into one check, with 25/75 splits). It is only after morph that we learn BB03 throws. I added a phase recently that adjusts the likelihoods of edges into throw blocks once we know they throw, but it only runs for PGO scenarios; I suppose it has similar value in non-PGO scenarios.

@BoyBaykiller out of curiosity, are you running your program with tiered compilation disabled? If not, I'd be interested in seeing a JIT dump of this method from a local run to see why these edges aren't being adjusted. Thanks!

[amanasifkhalid]

I can confirm that running the new edge likelihood adjustment phase when tiering is disabled fixes the layout for the above program:

---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight   [IL range]   [jump]                            [EH region]        [flags]
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1    [000..004)-> BB04(1),BB03(0)         ( cond )                   i LIR
BB04 [0003]  1       BB01                  1    [013..022)                           (return)                   i LIR
BB03 [0002]  1       BB01                  0.50 [008..013)                           (throw )                   i LIR hascall gcsafe
---------------------------------------------------------------------------------------------------------------------------------------------------------------------

Block layout relies on edge weights rather than block weights to drive layout decisions, so the incorrect weight on BB03 doesn't trip it up. At some point (maybe .NET 11), I'd like to enable all the new profile maintenance code for non-PGO scenarios too so we can get this right, but I suspect having non-zero weights on throw blocks isn't all that detrimental, since many (most?) throw blocks contain little more than the throw statement.

[EgorBo]

@amanasifkhalid but this is still basically a .NET 10.0 regression, isn't? For e.g. NativeAOT since that one typically doesn't have any profile data

[amanasifkhalid]

@amanasifkhalid but this is still basically a .NET 10.0 regression, isn't?

It is, and I suspect #111253 is to blame. In Compiler::fgConvertBBToThrowBB, we used to always mark throw blocks as rarely-run, without tweaking the edge likelihoods into the throw block. This inconsistency didn't pose a problem in .NET 9, because we didn't run profile repair anywhere after morph. Now in PGO scenarios, we can rely on the post-morph edge likelihood adjustment phase to reduce flow into throw blocks, and for profile repair to update throw blocks' weights accordingly. In non-PGO scenarios, these throw blocks are left with whatever incoming flow they already had. If this flow isn't miniscule, block layout may not push the block to the end of the method.

Ideally, we'd rely on the new edge likelihood adjustment phase to fix this problem, but this isn't the most targeted fix. We could also mark new throw blocks as rarely-run in Compiler::fgConvertBBToThrowBB like we used to, but only when we don't have profile data. That should return us to .NET 9's layout while (maybe) having little impact on other phases.

[BoyBaykiller]

out of curiosity, are you running your program with tiered compilation disabled? If not, I'd be interested in seeing a JIT dump of this method from a local run to see why these edges aren't being adjusted

I am. With TC I get the correct ordering in Tier1: https://gist.github.com/BoyBaykiller/f486cdf2ffd0703041bc019c3ffe4c53

I can confirm that running the new edge likelihood adjustment phase when tiering is disabled fixes the layout for the above program

Is there some env var to enable this new phase?

Is #111253 the reason why the throw block gets bbWeight=0.0 in .NET 9 and bbWeight=0.5 on main?

[amanasifkhalid]

Thanks for sharing the JIT dumps!

Is there some env var to enable this new phase?

There isn't at the moment. My plan is for it to always run when optimizing, regardless of whether TC is enabled or not, but that probably won't make it in .NET 10.

In .NET 10, we refactored several front-end phases to ensure the profile data fed into the JIT is maintained throughout transformations -- for example, if a block no longer jumps to another block, the weight into the latter block needs to be reduced accordingly. The phase I mentioned above is part of that work, and all of this work currently applies to PGO scenarios. When the JIT doesn't have PGO data for a method (for example, if TC is disabled and the user didn't give us static PGO), this profile manipulation doesn't kick in. Part of the reason we're conservative at the moment is because we aren't all that confident in the execution counts the JIT makes up for such scenarios. This is something I want to address in .NET 11: We have plans to implement smarter heuristics for creating fake profile data in non-PGO scenarios, and turning on all the profile synthesis enhancements we made in .NET 10 for all scenarios. For .NET 10, that's not the fix I want to make (see below).

Is #111253 the reason why the throw block gets bbWeight=0.0 in .NET 9 and bbWeight=0.5 on main?

Yes, sort of. That change is why bbWeight is no longer set to zero for the throw block; the 0.5 value is just left over from whatever the JIT's early block weight heuristics came up with. Those heuristics run before morph, so they're operating on limited information; for example, we don't know if ThrowOutOfRangeException throws an exception until it's inlined, or if it isn't inlined, until morph notices it doesn't return and converts the calling block into a throw block.

I think the solution I want to proceed with is to bring back the morph heuristic for setting throw blocks' weights to zero, but only in non-PGO scenarios. When we don't have PGO data, we're making the profile data up as we go, so I don't see an issue with doing the same here. I'd expect most of the churn from this change to be from throw blocks being moved out-of-line in the code layout, which is largely a return to .NET 9's behavior.

[amanasifkhalid]

With the changes in #117692, the layout is fixed for me:

*************** Starting PHASE Optimize layout
*************** In fgSearchImprovedLayout()

Initial BasicBlocks
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight   [IL range]   [jump]                            [EH region]        [flags]
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1    [000..004)-> BB04(0.25),BB03(0.75)   ( cond )                   i LIR
BB03 [0002]  1       BB01                  0    [008..013)                           (throw )                   i LIR rare hascall gcsafe
BB04 [0003]  1       BB01                  1    [013..022)                           (return)                   i LIR
---------------------------------------------------------------------------------------------------------------------------------------------------------------------

Not enough blocks to partition anything. Skipping reordering.
Reordering block list

*************** Finishing PHASE Optimize layout
Trees after Optimize layout

---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight   [IL range]   [jump]                            [EH region]        [flags]
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1    [000..004)-> BB04(0.25),BB03(0.75)   ( cond )                   i LIR
BB04 [0003]  1       BB01                  1    [013..022)                           (return)                   i LIR
BB03 [0002]  1       BB01                  0    [008..013)                           (throw )                   i LIR rare hascall gcsafe
---------------------------------------------------------------------------------------------------------------------------------------------------------------------