JIT: Remove BBF_NONE_QUIRK check when optimizing branch to empty unconditional

[amanasifkhalid]

Part of #95998. Removing the JumpsToNext check altogether seems to affect loop cloning, and also exposes a rare pattern where bDest's jump target is bDest, and we enter an infinite loop where we first try to optimize block -> bDest -> bDestTarget, then we try to optimize block -> bDestTarget -> bDest, etc. Just compacting bDest later on seems easier, and if we are able to compact it, we end up with block -> bDestTarget.

If this PR and #99783 are both merged, we can (finally) get rid of BBF_NONE_QUIRK.

[dotnet-policy-service]

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

[amanasifkhalid]

cc @dotnet/jit-contrib, @AndyAyersMS PTAL. Diffs

[AndyAyersMS]

exposes a rare pattern where bDest's jump target is bDest

Don't we already screen for this?

                if (bDest->KindIs(BBJ_ALWAYS) && !bDest->TargetIs(bDest) && // special case for self jumps
                    bDest->isEmpty())

[AndyAyersMS]

Changes look like an improvement, but I'm still puzzled why we need the JumpsToNext part.

I guess you are seeing cases where there is a two-block infinite loop where both blocks are empty, and the optimization just cycles around the loop trying to find the end. But it seems like we could have this kind of structure even w/o them being adjacent.

The right fix might be to add cycle detection of some kind...?

[amanasifkhalid]

Don't we already screen for this?

Sorry, I misspoke -- the problematic pattern is when bDest's jump target's jump target is bDest.

I guess you are seeing cases where there is a two-block infinite loop where both blocks are empty, and the optimization just cycles around the loop trying to find the end

That's it. In other words, the pattern is a cycle like bDest -> bDestTarget -> bDest.

The right fix might be to add cycle detection of some kind...?

I initially tried that, and it seemed to reduce the amount of loop cloning we do in some collections. If you'd like, I can open a follow-up PR with the cycle detection to show you the diffs.

[amanasifkhalid]

Both CI failures look like #99725.

[amanasifkhalid]

I initially tried that, and it seemed to reduce the amount of loop cloning we do in some collections.

Looking at the JIT dumps for affected methods, I suspect fgOptimizeBranchToEmptyUnconditional's logic for updating block weights is to blame for the diffs in loop cloning. I wonder if using likely weights fixes this...

[amanasifkhalid]

I wonder if using likely weights fixes this...

This fixed some instances, though not all of them. In libraries_tests.run, we see some increased loop cloning, and some decreased loop cloning. @AndyAyersMS do you want me to open a follow-up PR for this, or should we avoid messing with loop cloning?

[AndyAyersMS]

The post-merge comments ^^ are for a version with cycle detection?

I think we should try and get the block weights correct, and if that changes cloning, I'm ok with it. At some point (perhaps soon) we need to look harder at the cloning heuristics. In particular optCheckLoopCloningGDVTestProfitable should be using edge likelihoods and not block weights.

[amanasifkhalid]

Yes, that's all with cycle detection.

I think we should try and get the block weights correct, and if that changes cloning, I'm ok with it. At some point (perhaps soon) we need to look harder at the cloning heuristics. In particular optCheckLoopCloningGDVTestProfitable should be using edge likelihoods and not block weights.

Sounds like a good plan. Maybe we can bookmark the cycle detection work and come back to this after we've made some progress on block weights?