JIT: more fixes for VN loop dependence tracking by AndyAyersMS · Pull Request #56184 · dotnet/runtime

AndyAyersMS · 2021-07-22T20:53:04Z

Specify Overwrite when setting loop dependence map entries, as we may
refine the initial result.

Extract loop dependence of VNF_PhiMemoryDef.

Fixes new case noted in #55936, and 13/16 or so other cases Jakob sent
me privately. Also update a comment and fix tests to work better with
jitstress per other notes on that PR.

Specify `Overwrite` when setting loop dependence map entries, as we may refine the initial result. Fixes dotnet#56174. Extract loop dependence of `VNF_PhiMemoryDef`. Fixes new case noted in dotnet#55936, and 13/16 or so other cases Jakob sent me privately. Also update a comment and fix tests to work better with jitstress per other notes on that PR.

AndyAyersMS · 2021-07-22T20:53:23Z

cc @jakobbotsch @dotnet/jit-contrib

No spmi diffs.

briansull

Looks Good

AndyAyersMS · 2021-07-23T01:52:18Z

@jakobbotsch by my accounting this fixes all the test cases you sent me except for these 3:

11656046881568048475.cs
2692166044414030864.cs 
11738605779775786426.cs (hits jit assert)

The first two look like cases that should be handled by my fixes, so I will need to dig into them to see what is going wrong. In both cases the write/read sequence happens after a nested loop.

If a loop is removed (because of unrolling) then the loop dependence tracking introduced in dotnet#55936 and dotnet#56184 may not properly update. So when a loop is removed, walk up the chain of parent loops looking for one that is not removed, and record the dependence on that parent. Addresses last part of dotnet#54118.

…56436) If a loop is removed (because of unrolling) then the loop dependence tracking introduced in #55936 and #56184 may not properly update. So when a loop is removed, walk up the chain of parent loops looking for one that is not removed, and record the dependence on that parent. Addresses last part of #54118.

AndyAyersMS · 2021-07-28T20:23:31Z

The assert in that other test:

corerun.exe 11738605779775786426.exe

Assert failure(PID 20200 [0x00004ee8], Thread: 24136 [0x5e48]): Assertion failed 'i < BitSetTraits::GetSize(env)' 
   in 'Program:Main()' during 'Compute blocks reachability' (IL size 41)

AndyAyersMS · 2021-07-28T20:40:47Z

Looks like a flow opts bug, we end up not updating the pred list for BB08 properly and this ultimately leaves a dangling reference from BB07 to BB08 which causes the assert above.

*************** In fgUpdateFlowGraph()
Before updating the flow graph:

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight    lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1       [000..007)-> BB10 (always)                     i 
BB02 [0001]  1       BB10                  1       [007..00E)-> BB07 ( cond )                     i bwd bwd-target 
BB03 [0002]  1       BB02                  1       [00E..010)-> BB04 (always)                     keep i bwd 
BB04 [0009]  1       BB03                  1       [???..???)-> BB06 (always)                     i internal 
BB05 [0010]  0                             1       [???..???)-> BB09 (always)                     i internal 
BB06 [0003]  1       BB04                  1       [010..013)-> BB09 (always)                     keep i bwd 
BB07 [0004]  1       BB02                  1       [013..01A)-> BB09 ( cond )                     i bwd 
BB08 [0005]  1       BB07                  1       [01A..020)                                     i bwd 
BB09 [0006]  4       BB05,BB06,BB07,BB08   1       [020..024)                                     i bwd 
BB10 [0007]  2       BB01,BB09             1       [024..028)-> BB02 ( cond )                     i bwd 
BB11 [0008]  1       BB10                  1       [028..029)        (return)                     i 
-----------------------------------------------------------------------------------------------------------------------------------------


Removing unconditional jump to next block (BB03 -> BB04) (converted BB03 to fall-through)

Compacting blocks BB03 and BB04:
*************** In fgDebugCheckBBlist

Optimizing a jump to an unconditional jump (BB03 -> BB06 -> BB09)
Setting edge weights for BB03 -> BB09 to [0 .. 3.402823e+38]
fgRemoveBlock BB05

Removing unreachable BB05

Compacting blocks BB08 and BB09:
Second block has multiple incoming edges
Setting edge weights for BB03 -> BB08 to [0 .. 3.402823e+38]
Setting edge weights for BB06 -> BB08 to [0 .. 3.402823e+38]
*************** In fgDebugCheckBBlist

Moving BB07 after BB03 to enable reversal
New Basic Block BB12 [0011] created.
Setting edge weights for BB07 -> BB12 to [0 .. 3.402823e+38]

Reversing a conditional jump around an unconditional jump (BB02 -> BB07, BB03 -> BB08)
Setting edge weights for BB02 -> BB08 to [0 .. 3.402823e+38]

After reversing the jump:

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight    lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1       [000..007)-> BB10 (always)                     i 
BB02 [0001]  1       BB10                  1       [007..00E)-> BB08 ( cond )                     i bwd bwd-target 
BB07 [0004]  1       BB02                  1       [013..01A)-> BB08 ( cond )                     i bwd 
BB12 [0011]  1       BB07                  1       [???..???)-> BB08 (always)                     internal 
BB06 [0003]  0                             1       [010..013)-> BB08 (always)                     keep i bwd 
BB08 [0005]  4       BB02,BB06,BB12(2)     1       [01A..024)                                     i bwd 
BB10 [0007]  2       BB01,BB08             1       [024..028)-> BB02 ( cond )                     i bwd 
BB11 [0008]  1       BB10                  1       [028..029)        (return)                     i 
-----------------------------------------------------------------------------------------------------------------------------------------

AndyAyersMS · 2021-07-28T21:48:40Z

The bug is here:

runtime/src/coreclr/jit/fgopt.cpp

Lines 5620 to 5629 in eb1e55e

    
           // Add fall through fixup block, if needed. 
        
           // 
        
           if ((bDest->bbJumpKind == BBJ_NONE) || (bDest->bbJumpKind == BBJ_COND)) 
        
           { 
        
               BasicBlock* const bFixup = fgNewBBafter(BBJ_ALWAYS, bDest, true); 
        
               bFixup->inheritWeight(bDestNext); 
        
               bFixup->bbJumpDest = bDestNext; 
        
               fgReplacePred(bDestNext, bDest, bFixup); 
        
               fgAddRefPred(bFixup, bDest); 
        
           }

fgReplacePred will replace all pred uses, when we only wanted to replace one of them

Compacting blocks BB08 and BB09:
Second block has multiple incoming edges
Setting edge weights for BB03 -> BB08 to [0 .. 3.402823e+38]
Setting edge weights for BB06 -> BB08 to [0 .. 3.402823e+38]
*************** In fgDebugCheckBBlist

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight    lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1       [000..007)-> BB10 (always)                     i 
BB02 [0001]  1       BB10                  1       [007..00E)-> BB07 ( cond )                     i bwd bwd-target 
BB03 [0002]  1       BB02                  1       [00E..010)-> BB08 (always)                     keep i bwd 
BB06 [0003]  0                             1       [010..013)-> BB08 (always)                     keep i bwd 
BB07 [0004]  1       BB02                  1       [013..01A)-> BB08 ( cond )                     i bwd 
BB08 [0005]  4       BB03,BB06,BB07(2)     1       [01A..024)                                     i bwd 
BB10 [0007]  2       BB01,BB08             1       [024..028)-> BB02 ( cond )                     i bwd 
BB11 [0008]  1       BB10                  1       [028..029)        (return)                     i 
-----------------------------------------------------------------------------------------------------------------------------------------

Moving BB07 after BB03 to enable reversal
New Basic Block BB12 [0011] created.
Setting edge weights for BB07 -> BB12 to [0 .. 3.402823e+38]

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight    lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1       [000..007)-> BB10 (always)                     i 
BB02 [0001]  1       BB10                  1       [007..00E)-> BB07 ( cond )                     i bwd bwd-target 
BB03 [0002]  1       BB02                  1       [00E..010)-> BB08 (always)                     keep i bwd 
BB07 [0004]  1       BB02                  1       [013..01A)-> BB08 ( cond )                     i bwd 
BB12 [0011]  1       BB07                  1       [???..???)-> BB08 (always)                     internal 
BB06 [0003]  0                             1       [010..013)-> BB08 (always)                     keep i bwd 
BB08 [0005]  4       BB03,BB06,BB12(2)     1       [01A..024)                                     i bwd 
BB10 [0007]  2       BB01,BB08             1       [024..028)-> BB02 ( cond )                     i bwd 
BB11 [0008]  1       BB10                  1       [028..029)        (return)                     i 
-----------------------------------------------------------------------------------------------------------------------------------------

Reversing a conditional jump around an unconditional jump (BB02 -> BB07, BB03 -> BB08)
Setting edge weights for BB02 -> BB08 to [0 .. 3.402823e+38]

After reversing the jump:

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight    lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1       [000..007)-> BB10 (always)                     i 
BB02 [0001]  1       BB10                  1       [007..00E)-> BB08 ( cond )                     i bwd bwd-target 
BB07 [0004]  1       BB02                  1       [013..01A)-> BB08 ( cond )                     i bwd 
BB12 [0011]  1       BB07                  1       [???..???)-> BB08 (always)                     internal 
BB06 [0003]  0                             1       [010..013)-> BB08 (always)                     keep i bwd 
BB08 [0005]  4       BB02,BB06,BB12(2)     1       [01A..024)                                     i bwd 
BB10 [0007]  2       BB01,BB08             1       [024..028)-> BB02 ( cond )                     i bwd 
BB11 [0008]  1       BB10                  1       [028..029)        (return)                     i 
-----------------------------------------------------------------------------------------------------------------------------------------

This is new code that I added, I'll open a fresh issue for this...

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 22, 2021

AndyAyersMS mentioned this pull request Jul 22, 2021

JIT: track memory loop dependence of trees during value numbering #55936

Merged

BruceForstall approved these changes Jul 22, 2021

View reviewed changes

briansull approved these changes Jul 22, 2021

View reviewed changes

jakobbotsch approved these changes Jul 23, 2021

View reviewed changes

AndyAyersMS merged commit b18ff29 into dotnet:main Jul 27, 2021

AndyAyersMS deleted the LoopMemoryDependenceVNFixes branch July 27, 2021 17:53

AndyAyersMS mentioned this pull request Jul 27, 2021

Test failure JIT/Performance/CodeQuality/Roslyn/CscBench/CscBench.sh #55162

Closed

AndyAyersMS mentioned this pull request Jul 27, 2021

JIT: properly update loop memory dependence when loops are removed #56436

Merged

AndyAyersMS mentioned this pull request Jul 28, 2021

JIT: flow opts does an incorrect pred list update #56495

Closed

ghost locked as resolved and limited conversation to collaborators Aug 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: more fixes for VN loop dependence tracking#56184

JIT: more fixes for VN loop dependence tracking#56184
AndyAyersMS merged 1 commit intodotnet:mainfrom
AndyAyersMS:LoopMemoryDependenceVNFixes

AndyAyersMS commented Jul 22, 2021

Uh oh!

AndyAyersMS commented Jul 22, 2021

Uh oh!

briansull left a comment

Uh oh!

AndyAyersMS commented Jul 23, 2021 •

edited

Loading

Uh oh!

AndyAyersMS commented Jul 28, 2021

Uh oh!

AndyAyersMS commented Jul 28, 2021

Uh oh!

AndyAyersMS commented Jul 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

AndyAyersMS commented Jul 22, 2021

Uh oh!

AndyAyersMS commented Jul 22, 2021

Uh oh!

briansull left a comment

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndyAyersMS commented Jul 28, 2021

Uh oh!

AndyAyersMS commented Jul 28, 2021

Uh oh!

AndyAyersMS commented Jul 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AndyAyersMS commented Jul 23, 2021 •

edited

Loading