Hoist the invariants out of multi-level nested loops

[kunalspathak]

This is an experiment to hoist expressions out of multi-level loops. Currently, we just hoist expressions one-level up because of which some invariants are evaluated repeatedly in outer loop.

The shortcoming is because of two reasons from what I understand. First, we always hoist outer loop and then go and try to hoist code from inner loop as seen in this comment:

runtime/src/coreclr/jit/optimizer.cpp

Lines 6202 to 6208 in 1a296c0

	// We really should consider hoisting from conditionally executed blocks, if they are frequently executed
	// and it is safe to evaluate the tree early.
	//
	// In particular if we have a loop nest, when scanning the outer loop we should consider hoisting from blocks
	// in enclosed loops. However, this is likely to scale poorly, and we really should instead start
	// hoisting inner to outer.
	//

The second part is, we only evaluate the blocks that are immediate dominators of the exit blocks and do not evaluate conditional blocks that are frequently executed.

To overcome both of these problems, I changed the ordering of hoisting from inner to outer. Thus, we will first hoist all the nested loops present inside the current loop before hoisting the code present in the current loop. Now, once we create the preheader from the code hoisted from nested loop, we need to make sure that the preheader is considered while evaluating if it can be hoisted even outside the parent loop. For that, I keep track of newly created preheaders and then add them in the list of blocks to be evaluated for hoisting. When the hoisting is checked for the parent loop, if the code inside the preheader (that was added for the invariant code present in nested loop) is also invariant, it will get hoisted outside the parent loop too.

I see nice diffs from this change:

Example 1 taken from #35735 (comment):

Example 2 taken from #61420 (comment):

Example 3 taken from #61420 (comment)

Example 4 taken from Imagesharp code in #61420 (comment)

Example 5 taken from #6666 (comment)

Contributes to #65342

TODO:

• With the inner->outer flow, revisit GetHoistedInCurLoop() and ExtractHoistedInCurLoop()
• The immediate problem of hoisting past an expression that can throw is solved. The recent commit a4772a08a11 handles it by keeping track of GT_COMMA nodes and see if any of the siblings that occur before the node that we want to hoist (in the program execution) throws exception. If yes, it will prohibit the node from hoisting. However, I noticed that there are few cases where today, we "aggressively hoist" node from the GT_COMMA node even if the earlier siblings (nodes that will execute before the loop in question) might throw exception. I need to check if the existing mechanism is correct and if yes, adjust my conditions added in a4772a08a11 to adapt to it, so we don't see any regressions.
• Are there any other conditions that we should consider to prohibit from "aggressive hoisting" that might introduce functional correctness issues? Verify with some counter examples.
• Currently the preheader of inner loops are visited last because I am using stack to gather them and we pop them in reverse order. Fix it so the preheaders are processed in execution order.

[ghost]

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

Issue Details

---

This is an experiment to hoist expressions out of multi-level loops. Currently, we just hoist expressions one-level up because of which some invariants are evaluated repeatedly in outer loop.

The shortcoming is because of two reasons from what I understand. First, we always hoist outer loop and then go and try to hoist code from inner loop as seen in this comment:

runtime/src/coreclr/jit/optimizer.cpp

Lines 6202 to 6208 in 1a296c0

	// We really should consider hoisting from conditionally executed blocks, if they are frequently executed
	// and it is safe to evaluate the tree early.
	//
	// In particular if we have a loop nest, when scanning the outer loop we should consider hoisting from blocks
	// in enclosed loops. However, this is likely to scale poorly, and we really should instead start
	// hoisting inner to outer.
	//

The second part is, we only evaluate the blocks that are immediate dominators of the exit blocks and do not evaluate conditional blocks that are frequently executed.

To overcome both of these problems, I changed the ordering of hoisting from inner to outer. Thus, we will first hoist all the nested loops present inside the current loop before hoisting the code present in the current loop. Now, once we create the preheader from the code hoisted from nested loop, we need to make sure that the preheader is considered while evaluating if it can be hoisted even outside the parent loop. For that, I keep track of newly created preheaders and then add them in the list of blocks to be evaluated for hoisting. When the hoisting is checked for the parent loop, if the code inside the preheader (that was added for the invariant code present in nested loop) is also invariant, it will get hoisted outside the parent loop too.

I see nice diffs from this change:

Example 1 taken from #35735 (comment):

Example 2 taken from #61420 (comment):

Author:	kunalspathak
Assignees:	kunalspathak
Labels:	area-CodeGen-coreclr
Milestone:	-

[kunalspathak]

@dotnet/jit-contrib , @BruceForstall , @AndyAyersMS - any feedback on the approach that I am taking here?

[AndyAyersMS]

@dotnet/jit-contrib , @BruceForstall , @AndyAyersMS - any feedback on the approach that I am taking here?

So you are enabling conditional hoisting but just from inner loop preheaders? Seems like if we are going to do this we ought to think more about enabling conditional hoisting in general. But this will put pressure on CSE/LSRA.

FWIW in previous compilers we would consider conditional hoisting based on profile data if the loop block execution frequency was an appreciable fraction of the entry block frequency. This will "work" in either inner->outer or outer->inner hosting approaches, and it may even be that outer->inner works better overall as even a thinly executed inner loop block may end up getting hoisted as the benefit gets multiplied the further you can hoist it.

I don't know if our profile maintenance gives us reasonable numbers for the preheader block weight. There are a few places that are involved -- first loop inversion has to impute some kind of zero-trip likelihood; second preheader creation has to impute the right weight for the preheader (which will in general not be the weight of either its predecessor or successor).

I also don't know if we do the right safety checks to allow conditional hoisting; I suspect we don't.

[kunalspathak]

So you are enabling conditional hoisting but just from inner loop preheaders? Seems like if we are going to do this we ought to think more about enabling conditional hoisting in general. But this will put pressure on CSE/LSRA.

Yes, my idea is to start with smaller scenarios and get them in for .NET 7. As you see in the examples, it catches the common cases (specifically things like hoisting the static variables addresses for arm64). In future releases, we can build more general conditional hoisting.

But this will put pressure on CSE/LSRA.

If I can careful hoist the frequently executing loop blocks, it might still prove beneficial in execution time and might cover the register pressure impact.

[kunalspathak]

@BruceForstall , to answer your question offline for the mul example above, the reason is pretty much what I was mentioning. When we check the things to be hoisted from outside the loop, we don't consider the block that contains the multiplication operation. Here are the blocks before loop hoisting happens with following loops:

L00, from BB02 to BB04 (Head=BB01, Entry=BB02, Exit=BB04), child loop = L01
L01, from BB03 to BB03 (Head=BB02, Entry=BB03, Exit=BB03, parent=L00)

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight    lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1       [000..006)-> BB05 ( cond )                     i 
BB02 [0001]  2       BB01,BB04             4     0 [006..00A)-> BB04 ( cond )                     i Loop Loop0 bwd bwd-target 
BB03 [0002]  2       BB02,BB03            16     1 [00A..019)-> BB03 ( cond )                     i Loop Loop0 bwd bwd-target align 
BB04 [0004]  2       BB02,BB03             4     0 [019..022)-> BB02 ( cond )                     i bwd 
BB05 [0006]  2       BB01,BB04             1       [022..024)        (return)                     i 
-----------------------------------------------------------------------------------------------------------------------------------------

We first try to hoist L00 and only consider blocks BB02 and BB04 while the mul operation is inside BB03.

Loop Nest L00
optHoistThisLoop for loop L00 <BB02..BB04>:
  Loop body does not contain a call
  Loop has single exit

  USEDEF  (4)={V03 V00 V01 V02}
  INOUT   (4)={V03 V00 V01 V02}
  LOOPVARS(4)={V03 V00 V01 V02}
  Only considering hoisting in blocks that dominate exit block BB04

    optHoistLoopBlocks BB02 (weight=  4   ) of loop L00 <BB02..BB04>
      [000012] not hoistable: variant child
      [000040] not hoistable: variant child

    optHoistLoopBlocks BB04 (weight=  4   ) of loop L00 <BB02..BB04>
      [000033] not hoistable: variant child
      [000009] not hoistable: variant child

Next, we inspect L01 and decide to hoist the operation from inside that loop to outside, but now it is too late to be hoisted further out.

Nested Loop L01
optHoistThisLoop for loop L01 <BB03..BB03>:
  Loop body does not contain a call
  Loop has single exit

  USEDEF  (3)={V03 V00 V01}
  INOUT   (4)={V03 V00 V01 V02}
  LOOPVARS(3)={V03 V00 V01}
  Only considering hoisting in blocks that dominate exit block BB03

    optHoistLoopBlocks BB03 (weight= 16   ) of loop L01 <BB03..BB03>
      [000023] not hoistable: unset

[kunalspathak]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[kunalspathak]

The outerloop surprisingly is green, but I did find an example where we would AV because of eager hoisting.

[MethodImpl(MethodImplOptions.NoInlining)]
static int Problem1_Av(bool c, int z, int[] a)
{
    int r = 0;
    for (int i = 0; i < 10; i++)
    {
        if (c)
        {
            for (int j = 0; j < 10; j++)
            {
                r += a[z];
            }
        }
    }
    return r;
}

Here, we hoist a[z] outside the outer-loop while the bounds check stays outside the inner-loop leading the AV.

G_M50606_IG01:
       push     rsi
       sub      rsp, 32
                                                ;; size=5 bbWeight=0    PerfScore 0.00
G_M50606_IG02:
       xor      eax, eax
       xor      r9d, r9d
       movzx    rcx, cl
       mov      r10d, edx
       mov      r10d, dword ptr [r8+4*r10+16]  ; <-- AV
                                                ;; size=16 bbWeight=1    PerfScore 3.00
G_M50606_IG03:
       test     ecx, ecx
       je       SHORT G_M50606_IG06
                                                ;; size=4 bbWeight=4    PerfScore 5.00
G_M50606_IG04:
       xor      r11d, r11d
       mov      esi, dword ptr [r8+8]
       cmp      edx, esi
       jae      SHORT G_M50606_IG08
       mov      esi, r10d
       align    [0 bytes for IG05]
                                                ;; size=14 bbWeight=2    PerfScore 7.50
G_M50606_IG05:
       add      eax, esi
       inc      r11d
       cmp      r11d, 10
       jl       SHORT G_M50606_IG05
                                                ;; size=11 bbWeight=16    PerfScore 28.00
G_M50606_IG06:
       inc      r9d
       cmp      r9d, 10
       jl       SHORT G_M50606_IG03
                                                ;; size=9 bbWeight=4    PerfScore 6.00
G_M50606_IG07:
       add      rsp, 32
       pop      rsi
       ret
                                                ;; size=6 bbWeight=1    PerfScore 1.75
G_M50606_IG08:
       call     CORINFO_HELP_RNGCHKFAIL
       int3

I will fix this.

[AndyAyersMS]

I also don't know if we do the right safety checks to allow conditional hoisting; I suspect we don't.

I did find an example where we would AV because of eager hoisting.

Thanks for looking into this.

[kunalspathak]

Failures are #67816 and dotnet/arcade#9208

[kunalspathak]

/azp run runtime-coreclr outerloop, Fuzzlyn, Antigen

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[kunalspathak]

Antigen and Fuzzlyn failures are unrelated

[kunalspathak]

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[kunalspathak]

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[kunalspathak]

Failures are #69154, #68530

[kunalspathak]

@dotnet/jit-contrib - This is ready for review.

[kunalspathak]

From #35735 (comment), InvariantInBothDependent:

 ; rsp based frame
 ; fully interruptible
 ; No PGO data
-; invoked as altjit
 ; Final local variable assignments
 ;
-;  V00 arg0         [V00,T03] (  5, 14   )     int  ->  rcx         single-def
+;  V00 arg0         [V00,T04] (  5,  5   )     int  ->  rcx         single-def
 ;  V01 loc0         [V01,T01] (  4, 34   )     int  ->  rax
-;  V02 loc1         [V02,T04] (  4, 13   )     int  ->  rdx
+;  V02 loc1         [V02,T03] (  4, 13   )     int  ->  rdx
 ;  V03 loc2         [V03,T00] (  4, 52   )     int  ->   r8
 ;* V04 loc3         [V04    ] (  0,  0   )     int  ->  zero-ref
 ;# V05 OutArgs      [V05    ] (  1,  1   )  lclBlk ( 0) [rsp+00H]   "OutgoingArgSpace"
-;  V06 cse0         [V06,T02] (  2, 20   )     int  ->   r9         "CSE - aggressive"
+;  V06 cse0         [V06,T02] (  2, 17   )     int  ->  rcx         "CSE - aggressive"
 ;
 ; Lcl frame size = 0

@@ -22,20 +21,20 @@ G_M26730_IG01:
 G_M26730_IG02:
        xor      eax, eax
        xor      edx, edx
-                                               ;; size=4 bbWeight=1    PerfScore 0.50
+       mov      r8d, ecx
+       imul     r8d, ecx
+       imul     ecx, r8d
+                                               ;; size=15 bbWeight=1    PerfScore 4.75
 G_M26730_IG03:
        xor      r8d, r8d
-       mov      r9d, ecx
-       imul     r9d, ecx
-       imul     r9d, ecx
        align    [0 bytes for IG04]
-                                               ;; size=14 bbWeight=4    PerfScore 18.00
+                                               ;; size=3 bbWeight=4    PerfScore 1.00
 G_M26730_IG04:
-       lea      eax, [r9+rax+1]
+       lea      eax, [rcx+rax+1]
        inc      r8d
        cmp      r8d, 10
        jl       SHORT G_M26730_IG04
-                                               ;; size=14 bbWeight=16    PerfScore 40.00
+                                               ;; size=13 bbWeight=16    PerfScore 40.00
 G_M26730_IG05:
        inc      edx
        cmp      edx, 10
@@ -45,6 +44,6 @@ G_M26730_IG06:
        ret
                                                ;; size=1 bbWeight=1    PerfScore 1.00

-; Total bytes of code 40, prolog size 0, PerfScore 69.50, instruction count 15, allocated bytes for code 40 (MethodHash=2ba89795) for method ConsoleApp15.loop_hoist:InvariantInBothDependent(int):int
+; Total bytes of code 39, prolog size 0, PerfScore 56.65, instruction count 15, allocated bytes for code 39 (MethodHash=2ba89795) for method ConsoleApp15.loop_hoist:InvariantInBothDependent(int):int
 ; ============================================================

[SingleAccretion]

@kunalspathak could you show an example of how this safety check works? It is not obvious from the code why we treat COMMAs here specially.

[kunalspathak]

The isCommaTree check is not really needed since we only set hasExcep if isCommaTree == true. I have also renamed the method to IsHoistableOverExcepSibling to make it more clear.

The idea is to not move [000332] before [000328] because if it is out of bounds, we would see the exception throw earlier or AV as in #68061 (comment).

               [000338] ---XG------                         *  COMMA     void  
     ( 12, 15) [000327] ---XG--H---                         +--*  COMMA     ref    <l:$258, c:$257>
     (  8, 11) [000328] ---X---H---                         |  +--*  BOUNDS_CHECK_Rng void   $245
     (  1,  1) [000329] -------H---                         |  |  +--*  CNS_INT   int    0 $c0
     (  3,  3) [000330] ---X---H---                         |  |  \--*  ARR_LENGTH int    $1c2
     (  1,  1) [000331] -------H---                         |  |     \--*  LCL_VAR   ref    V00 arg0         u:1 $100
     (  4,  4) [000332] n---G--H---                         |  \--*  IND       ref    <l:$256, c:$106>
     (  1,  2) [000333] -------H---                         |     \--*  ARR_ADDR  byref ref[] $82
     (  1,  2) [000334] -------N---                         |        \--*  ADD       byref  $2c0
     (  1,  1) [000335] -------H---                         |           +--*  LCL_VAR   ref    V00 arg0         u:1 $100
     (  1,  1) [000336] -------H---                         |           \--*  CNS_INT   long   16 $280
               [000337] -----------                         \--*  NOP       void  

[SingleAccretion]

Out-of-order hoisting, I see. If we are hosting trees, we must hoist all of the side-effecting ones (in proper execution order), or only hoist the ones without side effects (which I suppose what the logic intends to do).

I still have some questions:

1. Why do we presume this problem can arise for COMMAs only?
2. Why is the logic gated on visiting the current tree? The comment above mentions:

At this point the stack contains (in top to bottom order):
  ...
  - ancestors of the current node and some of their descendants

  The ancestors have not been visited yet in post order so they're not hoistable
  (and they cannot become hoistable because the current node is not) but some of
  their descendants may have already been traversed and be hoistable.

So if some of those "descendants of ancestors" are a mix of such hoistable-not-hoistable nodes (like [000328] vs [000332] that we have above), the same problem could, presumably, arise.

Finally, as a remark, I have noticed there is quite a devious problem in the tree above, and it is the fact there is not enough information in it to determine that there is interference: [000332] implicitly depends on being placed after the range check (it is why it is marked non-faulting). The only thing that saves us there is the fact we have GLOB_REF on it and the interference check is very conservative (GLOB_REF by itself doesn't imply any side effects, it just means the tree's value depends on the memory state, but VN already took care of that "in theory"...).

[kunalspathak]

Why do we presume this problem can arise for COMMAs only?

The problem does exist today as well. Today, we don't even check if we are hoisting past a tree that has side-effects (the example that you are stating). To me, GT_COMMA was the obvious one because of bounds check, but certainly if there are more examples where things can go wrong, we should address them here or in future PRs.

Why is the logic gated on visiting the current tree?

The same reason. We visit the stack in execution order (bottom to top, as mentioned in comments) and I wanted to make sure to not hoist children past its parents, if parents have side-effects. Let me know if you can think of a better way to validate this.

the same problem could, presumably, arise.

I noticed last week too while (in main) debugging something related where we hoist the checks ahead of side-effects but in that example, the range checks were getting eliminated by assertion prop and hence wouldn't have seen any functional difference.

[SingleAccretion]

Hmm, so these are essentially "quirks" ("known holes in the algorithm"). They need to be documented as such.

Let me know if you can think of a better way to validate this.

I would expect the "general" (conservative) logic to be something like:

bool canHoistExceptions = true; // We can hoist them as long as we haven't left any behind.

for (tree : stack)
{
    bool hoisted = false;

    if (tree->IsHoistable() && (canHoistExceptions || ((tree->gtFlags & GTF_ALL_EFFECT) == 0)))
    {
        hoisted = optHoistCandidate(...); // Can fail for budget reasons, etc
    }

    if (!hoisted && ((tree->gtFlags & GTF_EXCEPT) != 0))
    {
        canHoistExceptions = false;
    }
}

Not sure how many regressions that would run into.

[kunalspathak]

Not sure how many regressions that would run into.

There are lot and it touches lot of methods that contains single-loops too that this PR don't intend to touch and I am not even sure if there are functional issues in these. I will leave it for future PRs.

Benchmarks windows/x64:
363 total methods with Code Size differences (295 improved, 68 regressed), 58 unchanged.

[SingleAccretion]

Yeah, cannot say I didn't suspect something like that would turn up :). My only remaining request here is for comments then.

[kunalspathak]

#6639