JIT: fgRemoveBlock does not maintain edge weights

[jakobbotsch]

This code drops the old edge and weight on it:

runtime/src/coreclr/jit/fgbasic.cpp

Line 5375 in 21b4a85

fgRemoveRefPred(succBlock, block);

If we have blocks A -> B -> C, with an empty B, then we should end up with the same result whether we compact B into C or whether we just remove B. However, currently we drop the A -> B weight if we remove B:

BB02 [0085]  2       BB01,BB36             1    211  0 [???..???)-> BB13 ( cond )                     IBC internal Loop loopheader 
BB03 [0087]  1       BB02                  0.42  88  0 [01D..???)-> BB04 (always)                     IBC internal LoopPH q
BB04 [0002]  2       BB03,BB12             0.55 117  1 [01D..02D)-> BB41 ( cond )                     i IBC Loop bwd bwd-target loopheader 
...
Setting edge weights for BB02 -> BB03 to [88.48387 .. 88.48387]
...
fgRemoveBlock BB03, unreachable=false
Removing empty BB03
Setting edge weights for BB02 -> BB04 to [0 .. 117] // not the right weight

When removing B, should the weight on A -> C be inherited from A -> B?

There's probably a ton of these in the JIT, but I figured I would open an issue since I saw it be the cause of diffs. (Also, maybe the fact that B -> C is not already the same as A -> B is a problem in maintenance in and of itself, so maybe the issue exists elsewhere.)

out_base.txt (jitdump with fgRemoveBlock)
out.txt (jitdump with compaction)

[ghost]

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

Issue Details

---

This code drops the old edge and weight on it:

runtime/src/coreclr/jit/fgbasic.cpp

Line 5375 in 21b4a85

fgRemoveRefPred(succBlock, block);

If we have blocks A -> B -> C, with an empty B, then we should end up with the same result whether we compact B into C or whether we just remove B. However, currently we drop the A -> B weight if we remove B:

BB02 [0085]  2       BB01,BB36             1    211  0 [???..???)-> BB13 ( cond )                     IBC internal Loop loopheader 
BB03 [0087]  1       BB02                  0.42  88  0 [01D..???)-> BB04 (always)                     IBC internal LoopPH q
BB04 [0002]  2       BB03,BB12             0.55 117  1 [01D..02D)-> BB41 ( cond )                     i IBC Loop bwd bwd-target loopheader 
...
Setting edge weights for BB02 -> BB03 to [88.48387 .. 88.48387]
...
fgRemoveBlock BB03, unreachable=false
Removing empty BB03
Setting edge weights for BB02 -> BB04 to [0 .. 117] // not the right weight

When removing B, should the weight on A -> C be inherited from A -> B?

There's probably a ton of these in the JIT, but I figured I would open an issue since I saw it be the cause of diffs. (Also, maybe the fact that B -> C is not already the same as A -> B is a problem in maintenance in and of itself, so maybe the issue exists elsewhere.)

Author:	jakobbotsch
Assignees:	-
Labels:	area-CodeGen-coreclr, untriaged
Milestone:	-

[AndyAyersMS]

Not sure we can do much here just yet, since either the incoming profile is already consistent (which we can locally check) and so no adjustment should be needed, or else we're making it inconsistent which likely indicates a problem elsewhere.

Once we have consistency checking enabled deeper into the jit we should run across and fix those elsewhere cases and can add a suitable assert here.

[AndyAyersMS]

Going to move this out of 9.0, since it depends on other work that is now opportunistic.

[jakobbotsch]

Going to close this given Andy's comments above, since this should be naturally caught and resolved as consistency checking gets enabled for more and more phases.