[LoopSink] Exit loop finding BBs to sink into early when possible (NFC) #101115
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As noted in the comments, findBBsToSinkInto is O(UseBBs.size() * ColdLoopBBs.size()) A very large function with a huge loop was incurring a high compile time in this code. The size of the ColdLoopBBs set was over 14K. There is a limit on the size of the UseBBs set, but not the ColdLoopBBs (and adding a limit for the latter actually slowed down some later passes). This change exits the loop early once we detect that there is no further refinement possible for the BBsToSinkInto set. This is possible because the ColdLoopBBs set is sorted in ascending magnitude of frequency.
|
@llvm/pr-subscribers-llvm-transforms Author: Teresa Johnson (teresajohnson) ChangesAs noted in the comments, findBBsToSinkInto is A very large function with a huge loop was incurring a high compile time This change exits the loop early once we detect that there is no further Full diff: https://github.com/llvm/llvm-project/pull/101115.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/LoopSink.cpp b/llvm/lib/Transforms/Scalar/LoopSink.cpp
index 6eedf95e7575e..5c6ed8487bbd1 100644
--- a/llvm/lib/Transforms/Scalar/LoopSink.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopSink.cpp
@@ -144,7 +144,23 @@ findBBsToSinkInto(const Loop &L, const SmallPtrSetImpl<BasicBlock *> &UseBBs,
BBsToSinkInto.erase(DominatedBB);
}
BBsToSinkInto.insert(ColdestBB);
+ continue;
}
+ // Otherwise, see if we can stop the search through the cold BBs early.
+ // Since the ColdLoopBBs list is sorted in increasing magnitude of
+ // frequency the cold BB frequencies can only get larger. The
+ // BBsToSinkInto set can only get smaller and have a smaller
+ // adjustedSumFreq, due to the earlier checking. So once we find a cold BB
+ // with a frequency at least as large as the adjustedSumFreq of the
+ // current BBsToSinkInto set, the earlier frequency check can never be
+ // true for a future iteration. Note we could do check this more
+ // aggressively earlier, but in practice this ended up being more
+ // expensive overall (added checking to the critical path through the loop
+ // that often ended up continuing early due to an empty
+ // BBsDominatedByColdestBB set, and the frequency check there was false
+ // most of the time anyway).
+ if (adjustedSumFreq(BBsToSinkInto, BFI) <= BFI.getBlockFreq(ColdestBB))
+ break;
}
// Can't sink into blocks that have no valid insertion point.
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/2589 Here is the relevant piece of the build log for the reference: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As noted in the comments, findBBsToSinkInto is
O(UseBBs.size() * ColdLoopBBs.size())
A very large function with a huge loop was incurring a high compile time
in this code. The size of the ColdLoopBBs set was over 14K. There is a
limit on the size of the UseBBs set, but not the ColdLoopBBs (and adding
a limit for the latter actually slowed down some later passes).
This change exits the loop early once we detect that there is no further
refinement possible for the BBsToSinkInto set. This is possible because
the ColdLoopBBs set is sorted in ascending magnitude of frequency.
This cut down the LoopSinkPass time by around 33% (78s to just over 50s).