Perform reachability analysis before codegen #66967

kunalspathak · 2022-03-22T00:12:51Z

Perform reachability analysis one last time before doing code gen to eliminate unreachable blocks.

While investigating, #66578, I noticed that there was lot of dead blocks that were kept around and that keeps variables alive longer and creating unnecessary refpositions. Having extra refpositions is not problematic, but the liveness of variables was not tracked correctly because of unreachable blocks. The liveness of variables decides if an interval should be spilled or not. As seen in my analysis in #66578 (comment), there were 3 refpositions (def, use, use) created for the problematic variable, but because of missing liveness, both the uses were marked as lastUse leading to making the interval as inactive instead of spilled.

In this PR, I have added the computeReachability() phase after all the optimizations are done so it can remove any unreachable blocks. It performs the block renumbering, but since LSRA is sensitive to block renumbering (see #66994), I didn't want to see its impact as part of this change. Hence, I have added an option to skip the block renumbering in the final phase.

PIN numbers on windows/x64 SPMI

Collection	Base	Diff	% diff
libraries	1392303237458	1393889260100	0.11%
benchmarks	328547820836	328920196300	0.11%

Also emit the block size after every block in order to spot the code size diff at block level.

Fixes: #66578

ghost · 2022-03-22T00:12:58Z

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

Issue Details

Perform reachability analysis one last time before doing code gen to eliminate unreachable blocks.

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

kunalspathak · 2022-03-22T20:55:56Z

@dotnet/jit-contrib

AndyAyersMS

Are you actually removing the blocks anywhere?

fgComputeReachability is not particularly efficient (it blindly unions all preds instead of just the ones with updates).

It is overkill for what I think you are looking for here -- you only want to know if a block A is reachable from method entry, not whether block A is reachable from block B.

fgComputeReachability also updates the gc safe block flags, which, while likely harmless is probably not something we want to be doing.

So I think you can get this information more cheaply and with less collateral impact.

AndyAyersMS · 2022-03-22T21:00:58Z

src/coreclr/jit/compiler.cpp

+        EnsureBasicBlockEpoch();
+        fgComputeReachability(/* computeDoms */ false, /* doRenumbering */ false);
+    };
+    DoPhase(this, PHASE_COMPUTE_REACHABILITY, computeReachability);


I would use a different local name and a different phase name (reusing a phase name likely messes up some stats computations).

BruceForstall · 2022-03-22T21:46:16Z

src/coreclr/jit/compiler.cpp

@@ -4769,7 +4769,8 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl

        // Compute reachability sets and dominators.
        //
-        DoPhase(this, PHASE_COMPUTE_REACHABILITY, &Compiler::fgComputeReachability);


Why does this need to change?

BruceForstall · 2022-03-22T21:47:15Z

src/coreclr/jit/emit.cpp

@@ -6556,7 +6556,8 @@ unsigned emitter::emitEndCodeGen(Compiler* comp,
 #ifdef DEBUG
        if (emitComp->opts.disAsm || emitComp->verbose)
        {
-            printf("\t\t\t\t\t\t;; bbWeight=%s PerfScore %.2f", refCntWtd2str(ig->igWeight), ig->igPerfScore);
+            printf("\t\t\t\t\t\t;;  size=%d   bbWeight=%s PerfScore %.2f", ig->igSize, refCntWtd2str(ig->igWeight),


nit: why so much whitespace?

BruceForstall · 2022-03-22T21:48:15Z

src/coreclr/jit/fgopt.cpp

+// Arguments:
+//    renumberingDone -- `true` if block renumbering was done.
+//
+void Compiler::fgComputeEnterBlocksSet(DEBUG_ARG1(const bool renumberingDone))


This is kind of annoying (adding an arg just for this). Why not just remove the offending assert instead? Doesn't seem like it has much value anyway.

BruceForstall · 2022-03-22T21:50:34Z

src/coreclr/jit/fgopt.cpp

@@ -543,10 +546,14 @@ bool Compiler::fgRemoveUnreachableBlocks()
 // Also, compute the list of return blocks `fgReturnBlocks` and set of enter blocks `fgEnterBlks`.
 // Delete unreachable blocks.
 //
+//  Arguments:
+//       computeDoms - Whether to compute doms or not.


It would be useful to understand (and add to this comment block) when renumbering is required, and why. And when it is not. Same for creating doms.

kunalspathak · 2022-03-22T22:51:19Z

Are you actually removing the blocks anywhere?

Yes, after it tracks the bbReach it uses it to determine (by referring the fgEnterBlks) whether to remove it or not, but I agree with you. I can do bbReach or equivalent calculation cheaper. Let me work on it.

runtime/src/coreclr/jit/fgopt.cpp

Lines 455 to 464 in 8b42dff

    
                       if (!BlockSetOps::IsEmptyIntersection(this, fgEnterBlks, block->bbReach)) 
        
                       { 
        
                           continue; 
        
                       } 
        
           #if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) 
        
                       if (!BlockSetOps::IsEmptyIntersection(this, fgAlwaysBlks, block->bbReach)) 
        
                       { 
        
                           continue; 
        
                       }

kunalspathak · 2022-03-22T23:02:16Z

@AndyAyersMS - Just to answer your original question, yes it does remove block in that method as seen in asmdiffs.

kunalspathak · 2022-03-23T07:18:36Z

I have verified the diffs between the new implementation and previous (exhaustive) implementation and there is no diff.

kunalspathak · 2022-03-24T18:18:59Z

Ping.

AndyAyersMS

I think the approach is good.

I have one question and also wondered if you had new TP measurements.

src/coreclr/jit/fgopt.cpp

kunalspathak · 2022-03-24T22:14:13Z

I guess there are superpmi replay for linux-arm failures that I need to look into.

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-66967-merge-88ab271474c44ff7ab/unix-arm-1/1/console.33965e13.log?sv=2019-07-07&se=2022-04-12T07%3A25%3A15Z&sr=c&sp=rl&sig=K9oClcSFxxSXGc4cuE6aKs4EUpOU9DzkcMyRZ6OKLWM%3D

src/coreclr/jit/fgopt.cpp

kunalspathak · 2022-03-28T23:26:20Z

Latest pin numbers on superpmi libraries_pmi collection:

Before: 327744184719
After: 328136546105
Diff: 0.12%

src/coreclr/jit/fgopt.cpp

kunalspathak · 2022-03-29T02:24:24Z

failure related to #59542

EgorBo · 2022-04-07T17:00:27Z

Improvements in System.Text.Json.Tests.Perf_Booleans dotnet/perf-autofiling-issues#4364 on x64
dotnet/perf-autofiling-issues#4284

kunalspathak added 3 commits March 21, 2022 11:59

Add size

f2c55ec

Remove unreachable blocks

fabc5e4

Ensure epoch

69e18df

ghost assigned kunalspathak Mar 22, 2022

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 22, 2022

Add a condition in asserT

27f3b0b

This was referenced Mar 22, 2022

ThreadPoolTests.CooperativeBlockingCanCreateThreadsFaster timed out #64964

Closed

Threadpool test CooperativeBlockingCanCreateThreadsFaster failing on Mac #66852

Closed

kunalspathak mentioned this pull request Mar 22, 2022

Revisit LSRA block sequence strategy #66994

Open

Add test case

dd47c1f

kunalspathak marked this pull request as ready for review March 22, 2022 20:55

AndyAyersMS reviewed Mar 22, 2022

View reviewed changes

BruceForstall requested changes Mar 22, 2022

View reviewed changes

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 22, 2022

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 22, 2022

kunalspathak added 2 commits March 22, 2022 20:56

Another shorter implementation

dd9a798

Add DeadBlock elimination

1d07ea0

AndyAyersMS reviewed Mar 24, 2022

View reviewed changes

src/coreclr/jit/fgopt.cpp Outdated Show resolved Hide resolved

BruceForstall reviewed Mar 25, 2022

View reviewed changes

fgDomsComputed adjusted

3785f23

BruceForstall approved these changes Mar 28, 2022

View reviewed changes

src/coreclr/jit/fgopt.cpp Outdated Show resolved Hide resolved

Review comment

0e37857

runfoapp bot mentioned this pull request Mar 29, 2022

System.Threading.Tests.PeriodicTimerTests.PeriodicTimer_ActiveOperations_TimerRooted failing in CI #59542

Closed

kunalspathak merged commit d2d2281 into dotnet:main Mar 29, 2022

kunalspathak deleted the unreachable_blocks branch March 29, 2022 16:19

This was referenced Apr 22, 2022

Test failure JIT\\opt\\OSR\\shortenregisteredlocal\\shortenregisteredlocal.cmd #68306

Closed

Dead cloned loops are not removed #5820

Closed

ghost locked as resolved and limited conversation to collaborators May 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform reachability analysis before codegen #66967

Perform reachability analysis before codegen #66967

kunalspathak commented Mar 22, 2022 •

edited

Loading

ghost commented Mar 22, 2022

kunalspathak commented Mar 22, 2022

AndyAyersMS left a comment

AndyAyersMS Mar 22, 2022

BruceForstall Mar 22, 2022

BruceForstall Mar 22, 2022

BruceForstall Mar 22, 2022

BruceForstall Mar 22, 2022

kunalspathak commented Mar 22, 2022

kunalspathak commented Mar 22, 2022

kunalspathak commented Mar 23, 2022

kunalspathak commented Mar 24, 2022

AndyAyersMS left a comment

kunalspathak commented Mar 24, 2022

kunalspathak commented Mar 28, 2022

kunalspathak commented Mar 29, 2022

EgorBo commented Apr 7, 2022 •

edited

Loading

Perform reachability analysis before codegen #66967

Perform reachability analysis before codegen #66967

Conversation

kunalspathak commented Mar 22, 2022 • edited Loading

ghost commented Mar 22, 2022

kunalspathak commented Mar 22, 2022

AndyAyersMS left a comment

Choose a reason for hiding this comment

AndyAyersMS Mar 22, 2022

Choose a reason for hiding this comment

BruceForstall Mar 22, 2022

Choose a reason for hiding this comment

BruceForstall Mar 22, 2022

Choose a reason for hiding this comment

BruceForstall Mar 22, 2022

Choose a reason for hiding this comment

BruceForstall Mar 22, 2022

Choose a reason for hiding this comment

kunalspathak commented Mar 22, 2022

kunalspathak commented Mar 22, 2022

kunalspathak commented Mar 23, 2022

kunalspathak commented Mar 24, 2022

AndyAyersMS left a comment

Choose a reason for hiding this comment

kunalspathak commented Mar 24, 2022

kunalspathak commented Mar 28, 2022

kunalspathak commented Mar 29, 2022

EgorBo commented Apr 7, 2022 • edited Loading

kunalspathak commented Mar 22, 2022 •

edited

Loading

EgorBo commented Apr 7, 2022 •

edited

Loading