[PATCH] D53887: [HotColdSplitting] Outline more than once per function

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

In https://reviews.llvm.org/D53887#1290587, @vsk wrote:

> In https://reviews.llvm.org/D53887#1290584, @junbuml wrote:
>
> > I guess it would be good to give some stress test on this pass to see if there is any hidden bug by relaxing conditions in unlikelyExecuted().  As we treat more blocks as cold without being limited on unlikelyExecuted(), we maybe able to expose hidden issues with it.
>
>
> That's a great idea. I'll do that now with the test-suite + externals. FWIW, I also built all of iOS with this pass enabled, and the only compiler crash I found was https://bugs.llvm.org/show_bug.cgi?id=39564. I'm still investigating possible miscompiles.


So, I defined unlikelyExecuted(BB) = true, and found a bug while running through the test-suite. The issue is that two do two DFS's (one on predecessor blocks, one on successor blocks). The second DFS can mark a block already marked by the first DFS. In practice this isn't a problem, because CodeExtractor maintains a set of blocks. But it's weird to have duplicate blocks in the extraction list, and it's a simple issue to fix.

Other than that, I found no miscompiles. The test suite passed cleanly.

In https://reviews.llvm.org/D53887#1290587, @vsk wrote:

> In https://reviews.llvm.org/D53887#1290584, @junbuml wrote:
>
> > I guess it would be good to give some stress test on this pass to see if there is any hidden bug by relaxing conditions in unlikelyExecuted().  As we treat more blocks as cold without being limited on unlikelyExecuted(), we maybe able to expose hidden issues with it.
>
>
> That's a great idea. I'll do that now with the test-suite + externals. FWIW, I also built all of iOS with this pass enabled, and the only compiler crash I found was https://bugs.llvm.org/show_bug.cgi?id=39564. I'm still investigating possible miscompiles.


So, I defined `bool Cold = !pred_empty(BB)`, and this resulted in 29MB of text being outlined in test suite (out of 43MB total). I found a small issue in https://reviews.llvm.org/D54189, but no actual miscompilations. All tests passed and matched the reference output.


https://reviews.llvm.org/D53887