JIT: Remove some `fgRenumberBlocks` calls

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.

https://docs.microsoft.com/dotnet/core/

MIT License

15.41k stars 4.76k forks source link

JIT: Remove some `fgRenumberBlocks` calls #110026

Open amanasifkhalid opened 1 day ago

amanasifkhalid commented 1 day ago

Part of #107749. While we cannot get rid of fgRenumberBlocks altogether yet, this gets us close. The JIT still as a few hard dependencies on bbNums reflecting relative lexical order:

If the CFG has loops, optSetBlockWeights will use bbNums for naive loop backedge detection.
Loop inversion also uses bbNums in lieu of flow edges to guess at a loop's shape.
Loop duplication helpers iterate the loop's blocks in lexical order, which requires ordered bbNums. As such, I've left the fgRenumberBlocks calls in loop cloning/unrolling in-place for now.

There's also a soft dependency on bbNum ordering in fgUpdateFlowGraph that can trigger diffs. These diffs were quite small locally, so I think we can tolerate them for now.

dotnet-policy-service[bot] commented 1 day ago

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

amanasifkhalid commented 9 hours ago

cc @dotnet/jit-contrib, @jakobbotsch PTAL. Small diffs. The Instrumented Tier0 diffs stem from the fact that bbNums are used in key computations by EfficientEdgeCountBlockToKey here, so the lack of renumbering is churning the instrumentation schema a bit. The resultant codegen diffs are from the address of the profile data now fitting in 32/64 bits, instead of 64/32 bits. Thanks!

AndyAyersMS commented 8 hours ago

The Instrumented Tier0 diffs stem from the fact that bbNums are used in key computations by EfficientEdgeCountBlockToKey

We can change this to use bbID too. It may break reading in static PGO data for a while but so does changing the values of bbNum. Note when we get around to https://github.com/dotnet/runtime/issues/44372, instrumented inlinees will need to adjust this value to make it inlinee-relative (as if we'd instrumented the method as root).

amanasifkhalid commented 8 hours ago

We can change this to use bbID too.

Do we have anything to gain from switching this over to bbID? Assuming bbNum continues to stick around, it seems like it would be easier to keep using it for the block key since it's already inlinee-relative, right?

AndyAyersMS commented 5 hours ago

We can change this to use bbID too.

Do we have anything to gain from switching this over to bbID? Assuming bbNum continues to stick around, it seems like it would be easier to keep using it for the block key since it's already inlinee-relative, right?

bbID seems more stable, but I suppose as long as we're not ever tempted to add renumbering early, this can stay as is.