JIT: Remove BBJ_ALWAYS fall-through check in Compiler::fgFindInsertPoint

[amanasifkhalid]

Part of #95998. This reverts the behavior of Compiler::fgFindInsertPoint to how it was before BBJ_NONE was removed. In order to remove the BBF_NONE_QUIRK check, I initially tried to adapt fgFindInsertPoint to not insert new blocks after BBJ_ALWAYS blocks that jump to the next block (i.e. mimicking BBJ_NONE behavior), but fgFindInsertPoint is called in numerous phases (importation, block reordering, etc.) where block layout is not finalized, so it might be premature to try to keep BBJ_ALWAYS blocks contiguous with their jump targets. I'd like to try our previous approach of checking only for implicit fall-through instead. This produced dramatic diffs on win-x64 locally; every collection was still a significant net size improvement, and most regressions are due to changes in block ordering resulting in more/longer jumps. In one method, this change enabled the JIT to do more loop cloning, resulting in massive size increases. I'm opening this now for visibility, and will update with explanations of example diffs.

[ghost]

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

Issue Details

---

Part of #95998. This reverts the behavior of Compiler::fgFindInsertPoint to how it was before BBJ_NONE was removed. In order to remove the BBF_NONE_QUIRK check, I initially tried to adapt fgFindInsertPoint to not insert new blocks after BBJ_ALWAYS blocks that jump to the next block (i.e. mimicking BBJ_NONE behavior), but fgFindInsertPoint is called in numerous phases (importation, block reordering, etc.) where block layout is not finalized, so it might be premature to try to keep BBJ_ALWAYS blocks contiguous with their jump targets. I'd like to try our previous approach of checking only for implicit fall-through instead. This produced dramatic diffs on win-x64 locally; every collection was still a significant net size improvement, and most regressions are due to changes in block ordering resulting in more/longer jumps. In one method, this change enabled the JIT to do more loop cloning, resulting in massive size increases. I'm opening this now for visibility, and will update with explanations of example diffs.

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

[jakobbotsch]

In one method, this change enabled the JIT to do more loop cloning, resulting in massive size increases.

If you see these with any sort of frequency you might consider waiting until I've fully switched loop cloning to the new loop representation. I wouldn't expect this change to impact the new loop finding or loop cloning after that is done.

[amanasifkhalid]

If you see these with any sort of frequency you might consider waiting until I've fully switched loop cloning to the new loop representation.

Thank you for pointing that out. For this change, we see increased loop cloning in a few methods, and for the other nontrivial BBF_NONE_QUIRK removal (in Compiler::fgUpdateFlowGraph), we see reduced loop cloning in a few methods. I'll hold off on these removals until after you've wrapped up the loop representation work, and see if these large diffs still repro.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.