JIT: Don't clone or unroll cold loops

[amanasifkhalid]

Follow-up to #115734 (comment).

[Copilot]

Pull Request Overview

This PR prevents unnecessary JIT work by skipping loop unrolling and cloning for cold loops, as indicated by the updated logic in both functions.

• Added a cold loop check in the loop unrolling function in optimizer.cpp.
• Added the same cold loop check in the loop cloning function in loopcloning.cpp.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/coreclr/jit/optimizer.cpp	Added early exit for unrolling of cold loops
src/coreclr/jit/loopcloning.cpp	Added early exit for cloning of cold loops

[dotnet-policy-service]

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

[amanasifkhalid]

@dotnet/jit-contrib PTAL. Diffs show some size increases in cases where cloning previously removed a bounds check, and RBO later removed the second loop body; now, we leave the bounds check in-place. For PGO collections, there are few PerfScore diffs since we're targeting cold code, and we try to reestablish profile consistency right before loop opts. If we're ok with the behavior diverging, I'm ok with reenabling cloning/unrolling cold loops for non-PGO scenarios since we aren't as confident about what is actually cold. Thanks!

[EgorBo]

I think we need some sort of a knob to replace all cold blocks with some minimal weight so none of the opts give up on them (with isRunRarely) and check that on large benchmarks. I suspect we loose a bit of perf by giving up on what we think is cold, while in fact it is not (e.g. unreliable static pgo, lack of context-oriented dynamic PGO that bakes weights for the first use during the start)

But the general question is: should we turn off opts for cold parts if the TP wins aren't too big?

[amanasifkhalid]

I think we need some sort of a knob to replace all cold blocks with some minimal weight so none of the opts give up on them

I think that would be interesting to try. I think the simplest way to quickly try this is to make the threshold isRunRarely checks some negative weight we never expect a block to have. As we can see in the diffs, bailing on loop cloning can inadvertently increase cold code size, and I've found cases where not inverting a cold loop can pessimize RBO and also lead to increased code size, though I don't think we can predict whether these opts will be size-increasing or decreasing so early.

But the general question is: should we turn off opts for cold parts if the TP wins aren't too big?

If the optimization is typically size-increasing, and we are confident the code is cold, I think we ought to disable it regardless of the TP wins. Without hot/cold splitting, this means being able to place more hot code on each memory page. With my current splitting prototype, it also means being able to fit more hot code in each code heap.

[amanasifkhalid]

Leaving this open until after the Preview 5 snap, fyi

[amanasifkhalid]

/ba-g build timeouts, and jit-format broken