Bug fixes from dead code elimination

[kunalspathak]

Fixes 2 issues from the dead code elimination changes earlier.

!foundDiff

This one was happening because we detect certain block is dead and remove the code inside it, but sometimes might decide to not delete the block itself. In those cases, we need to still make sure that we update liveness of variables. In #69421, I was only updating the liveness if we deleted the actual blocks.

The fix is to take into account if any block was unreachable and if yes, update the liveness info.

cookie != nullptr

This one was interesting. We have chain of try-finally that looks like this. During dead block elimination, we eliminate BB08 and BB09. However, we fail to detect that BB11 could be eliminated as well because the only block that reaches BB11 is BB09. Going forward, we add labels to genMarkLabelsForCodegen and since no block is jumping to BB11 , we do not add label to it, do not generate a dedicated IG for it and hence do not have bbEmitCookie.

BB08 [0016]  1  1    BB04                  1       [???..???)-> BB15 (callf ) T1                  i internal LIR 
BB09 [0017]  1  1    BB15                  1       [???..???)-> BB11 (ALWAYS) T1      }           i internal LIR KEEP 
BB10 [0021]  2       BB02,BB07             1       [033..03E)-> BB14 (always)                     keep i LIR cfb cfe 
BB11 [0018]  1       BB09                  1       [???..???)-> BB17 (callf )                     i internal LIR 
BB12 [0019]  1       BB17                  1       [???..???)-> BB13 (ALWAYS)                     i internal LIR KEEP 

The fix was to check if we made any reachable blocks dead and if yes, do another iteration. We would restrict the number of iterations to 3.

Fixes: #69659

[ghost]

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

Issue Details

---

Fixes 2 issues from the dead code elimination changes earlier.

!foundDiff

This one was happening because we detect certain block is dead and remove the code inside it, but sometimes might decide to not delete the block itself. In those cases, we need to still make sure that we update liveness of variables. In #69421, I was only updating the liveness if we deleted the actual blocks.

The fix is to take into account if any block was unreachable and if yes, update the liveness info.

cookie != nullptr

This one was interesting. We have chain of try-finally that looks like this. During dead block elimination, we eliminate BB08 and BB09. However, we fail to detect that BB11 could be eliminated as well because the only block that reaches BB11 is BB09. Going forward, we add labels to genMarkLabelsForCodegen and since no block is jumping to BB11 , we do not add label to it, do not generate a dedicated IG for it and hence do not have bbEmitCookie.

BB08 [0016]  1  1    BB04                  1       [???..???)-> BB15 (callf ) T1                  i internal LIR 
BB09 [0017]  1  1    BB15                  1       [???..???)-> BB11 (ALWAYS) T1      }           i internal LIR KEEP 
BB10 [0021]  2       BB02,BB07             1       [033..03E)-> BB14 (always)                     keep i LIR cfb cfe 
BB11 [0018]  1       BB09                  1       [???..???)-> BB17 (callf )                     i internal LIR 
BB12 [0019]  1       BB17                  1       [???..???)-> BB13 (ALWAYS)                     i internal LIR KEEP 

The fix was to check if we made any reachable blocks dead and if yes, do another iteration. We would restrict the number of iterations to 3.

Fixes: #69659

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

[kunalspathak]

@dotnet/jit-contrib

[AndyAyersMS]

The fix was to check if we made any reachable blocks dead and if yes, do another iteration. We would restrict the number of iterations to 3.

Finally chains can be arbitrarily long, so how does this keep us from getting into trouble? Seems like we either need to iterate until we've got rid of all unreachable blocks, or have some way to handle blocks that are unreachable but still in the bb chain.

[kunalspathak]

so how does this keep us from getting into trouble

True, and for now, I just added a check to not iterate more than 3 times.

Seems like we either need to iterate until we've got rid of all unreachable blocks

Which as you pointed can be problematic and long.

have some way to handle blocks that are unreachable but still in the bb chain.

Another alternative I tried was to re-populate the visitedBlocks list that will hopefully capture some of those dead blocks, but re-populating them again might be time consuming too and in common cases, might not give us much benefit. Additionally, it might still not get rid of all the blocks and will have to deal with them one at a time which I am trying to restrict with the iteration count.

[AndyAyersMS]

fgComputeReachability does something similar to what you are doing here, but just repeatedly calls fgRemoveUnreachableBlocks (with a limit of 10 enforced by noway).

If you changed the isBlockRemovable predicate to something like return (block->bbRefs == 0) || !isVisited I think you could follow that same pattern with less bookkeeping.

[AndyAyersMS]

fgComputeReachability does something similar to what you are doing here, but just repeatedly calls fgRemoveUnreachableBlocks (with a limit of 10 enforced by noway).

If you changed the isBlockRemovable predicate to something like return (block->bbRefs == 0) || !isVisited I think you could follow that same pattern with less bookkeeping.

[kunalspathak]

does something similar to what you are doing here

Right, I didn't want to go up to 10 iterations, and just soft break the loop. But I think we will still hit a problem with a weird case that has more than 3 chains of try-finally. So, I will keep the code similar to fgComputeReachability , iterate up to 10 times.

[AndyAyersMS]

LGTM

You have formatting to fix and some arm failures to look into.

[kunalspathak]

You have formatting to fix and some arm failures to look into.

Done.

[jakobbotsch]

Sorry for the late review, but GitHub_XYZ refers to the dotnet/coreclr repo, so this should have been named Runtime_69659. Also it would be nice to name the class something like Runtime_69659, it makes it easier to track the test down on failures.

It seems there are a few tests incorrectly filed under GitHub_XYZ tag, so maybe something to consider cleaning up as part of a future change.

[kunalspathak]

so this should have been named Runtime_69659

Ah, didn't realize that. I named them _1 and _2 because there were 2 separate issues that were filed in #69659. I think I forgot to have the right class name in one of those files.

[jakobbotsch]

Ah sorry, Runtime_69569_1 etc. sounds right then.