This is an archive of the discontinued LLVM Phabricator instance.

[SimpleLoopUnswitch] Skip non-trivial unswitching of cold loop nests
ClosedPublic

Authored by tejohnson on Mar 19 2023, 8:54 AM.

Details

Summary

This fixes a compile time issue due to guarding loop unswitching based
on whether the enclosing function is cold. That approach is very
inefficient in the case of large cold functions that contain numerous
loops, since the loop pass calls isFunctionColdInCallGraph once per
loop, and that function walks all BBs in the function (twice for Sample
PGO) looking for any non-cold blocks.

Originally, this code only checked if the current Loop's header was cold
(D129599). However, that apparently caused a slowdown on a SPEC
benchmark, and the example given was that of a cold inner loop nested in
a non-cold outer loop (see comments in D129599). The fix was to check if
the whole function is cold, done in D133275.

This is overkill, and we can simply check if the header of any loop in
the current loop's loop nest is non-cold (looking at both outer and
inner loops). This patch drops the compile time for a large module by
40% with this approach.

I also updated the test since it only had one cold loop in a non-cold
function, so that it instead had IR based off the example given in the
comments relating to the SPEC degradation in D129599. I confirmed that
the new version of the test fails with the original check done in
D129599 of only the current loop's header coldness.

Diff Detail

Event Timeline

tejohnson created this revision.Mar 19 2023, 8:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2023, 8:54 AM
tejohnson requested review of this revision.Mar 19 2023, 8:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2023, 8:54 AM

seems reasonable, just some nits

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
3506

within

llvm/test/Transforms/SimpleLoopUnswitch/PGO-nontrivial-unswitch2.ll
8–16

we should also have a test where both loops are cold and both are hot

24

can some of these attributes (and the test case in general) be cleaned up?

tejohnson marked 2 inline comments as done.Mar 20 2023, 8:40 AM
tejohnson added inline comments.
llvm/test/Transforms/SimpleLoopUnswitch/PGO-nontrivial-unswitch2.ll
8–16

PGO-nontrivial-unswitch.ll was already testing a cold loop in a cold function, but given that we are not looking for function coldness anymore, I changed that to be a version of this test case with a cold loop in a cold loop nest. I also added PGO-nontrivial-unswitch3.ll to test a non-cold loop in a non-cold loop nest.

24

I removed these attributes and tried to remove a bit more unneeded detail from the IR and metadata (had already had removed some unneeded metadata).

tejohnson updated this revision to Diff 506610.Mar 20 2023, 8:44 AM
tejohnson marked an inline comment as done.

Address comments

This revision is now accepted and ready to land.Mar 20 2023, 9:14 AM
This revision was landed with ongoing or failed builds.Mar 20 2023, 10:15 AM
This revision was automatically updated to reflect the committed changes.