[MLGO] Count LR Evictions Rather than Relying on Cascade #124440
Merged
Conversation
This file contains 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
boomanaiden154
requested review from
kongy,
mtrofin and
pirama-arumuga-nainar
|
@llvm/pr-subscribers-mlgo Author: Aiden Grossman (boomanaiden154) ChangesThis patch adjusts the mlregalloc-max-cascade flag (renaming it to mlregalloc-max-eviction-count) to actually count evictions rather than just looking at the cascade number. The cascade number is not very representative of how many times a LR has been evicted, which can lead to some problems in certain cases, where we might end up with many eviction problems where we have now masked off all the interferences and are forced to evict the candidate. This is probably what I should've done in the first place. No test case as this only shows up in quite large functions post ThinLTO and it would be hard to construct something that would serve as a nice regression test without being super brittle. I've tested this on the pathological cases that we have come across so far and it works. Fixes #122829 Full diff: https://github.com/llvm/llvm-project/pull/124440.diff 1 Files Affected:
diff --git a/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp b/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp
index 9c6487b40d6061..7a95c36f431e1a 100644
--- a/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp
+++ b/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp
@@ -63,11 +63,11 @@ static cl::opt<std::string> InteractiveChannelBaseName(
"outgoing name should be "
"<regalloc-evict-interactive-channel-base>.out"));
-static cl::opt<unsigned>
- MaxCascade("mlregalloc-max-cascade", cl::Hidden,
- cl::desc("The maximum number of times a live range can be "
- "evicted before preventing it from being evicted"),
- cl::init(20));
+static cl::opt<unsigned> MaxEvictionCount(
+ "mlregalloc-max-eviction-count", cl::Hidden,
+ cl::desc("The maximum number of times a live range can be "
+ "evicted before preventing it from being evicted"),
+ cl::init(20));
// Options that only make sense in development mode
#ifdef LLVM_HAVE_TFLITE
@@ -364,6 +364,8 @@ class MLEvictAdvisor : public RegAllocEvictionAdvisor {
using RegID = unsigned;
mutable DenseMap<RegID, LIFeatureComponents> CachedFeatures;
+
+ mutable std::unordered_map<unsigned, unsigned> VirtRegEvictionCounts;
};
#define _DECL_FEATURES(type, name, shape, _) \
@@ -657,7 +659,7 @@ bool MLEvictAdvisor::loadInterferenceFeatures(
// threshold, prevent the range from being evicted. We still let the
// range through if it is urgent as we are required to produce an
// eviction if the candidate is not spillable.
- if (IntfCascade >= MaxCascade && !Urgent)
+ if (VirtRegEvictionCounts[Intf->reg().id()] > MaxEvictionCount && !Urgent)
return false;
// Only evict older cascades or live ranges without a cascade.
@@ -803,6 +805,21 @@ MCRegister MLEvictAdvisor::tryFindEvictionCandidate(
}
assert(CandidatePos < ValidPosLimit);
(void)ValidPosLimit;
+
+ // Update information about how many times the virtual registers being
+ // evicted have been evicted.
+ if (CandidatePos == CandidateVirtRegPos) {
+ VirtRegEvictionCounts[VirtReg.reg()] += 1;
+ } else {
+ for (MCRegUnit Unit : TRI->regunits(Regs[CandidatePos].first)) {
+ LiveIntervalUnion::Query &Q = Matrix->query(VirtReg, Unit);
+ const auto &IFIntervals = Q.interferingVRegs(EvictInterferenceCutoff);
+ for (const LiveInterval *Intf : reverse(IFIntervals)) {
+ VirtRegEvictionCounts[Intf->reg()] += 1;
+ }
+ }
+ }
+
return Regs[CandidatePos].first;
}
|
|
I've had to add state to the eviction advisor with a new |
This patch adjusts the mlregalloc-max-cascade flag (renaming it to mlregalloc-max-eviction-count) to actually count evictions rather than just looking at the cascade number. The cascade number is not very representative of how many times a LR has been evicted, which can lead to some problems in certain cases, where we might end up with many eviction problems where we have now masked off all the interferences and are forced to evict the candidate. This is probably what I should've done in the first place. No test case as this only shows up in quite large functions post ThinLTO and it would be hard to construct something that would serve as a nice regression test without being super brittle. I've tested this on the pathological cases that we have come across so far and it works. Fixes llvm#122829
boomanaiden154
force-pushed
the
android-compile-time-fix-1-25-25
branch
from
8d26521 to
5947599
Compare
kongy
approved these changes
Jan 26, 2025
mtrofin
reviewed
Jan 27, 2025
mtrofin
reviewed
Jan 27, 2025
mtrofin
reviewed
Jan 27, 2025
mtrofin
approved these changes
Jan 27, 2025
You can test this locally with the following command:git-clang-format --diff c1ec5beb4ab36c2c4d99ed6d735d217e74364771 ff42e631e49efea69d553849763aaab1cc94eb45 --extensions cpp -- llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cppView the diff from clang-format here.diff --git a/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp b/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp
index a689abb7ac..c79127d6b8 100644
--- a/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp
+++ b/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp
@@ -673,7 +673,7 @@ bool MLEvictAdvisor::loadInterferenceFeatures(
// large amount of compile time being spent in regalloc. If we hit the
// threshold, prevent the range from being evicted. We still let the
// range through if it is urgent as we are required to produce an
- // eviction if the candidate is not spillable.
+ // eviction if the candidate is not spillable.
if (getEvictionCount(Intf->reg()) > MaxEvictionCount && !Urgent)
return false;
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/50/builds/9470 Here is the relevant piece of the build log for the reference |
boomanaiden154
added a commit
that referenced
this pull request
Jan 28, 2025
…4440)" This reverts commit 8cc83b6. This was causing builbot failures. https://lab.llvm.org/buildbot/#/builders/90/builds/4198 https://lab.llvm.org/buildbot/#/builders/110/builds/3616
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/12529 Here is the relevant piece of the build log for the reference |
boomanaiden154
added a commit
that referenced
this pull request
Jan 28, 2025
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.
This patch adjusts the mlregalloc-max-cascade flag (renaming it to mlregalloc-max-eviction-count) to actually count evictions rather than just looking at the cascade number. The cascade number is not very representative of how many times a LR has been evicted, which can lead to some problems in certain cases, where we might end up with many eviction problems where we have now masked off all the interferences and are forced to evict the candidate.
This is probably what I should've done in the first place. No test case as this only shows up in quite large functions post ThinLTO and it would be hard to construct something that would serve as a nice regression test without being super brittle. I've tested this on the pathological cases that we have come across so far and it works.
Fixes #122829