[RegAllocEvictAdvisor] Add minimum weight ratio heuristic. #98109
Conversation
| @@ -44,6 +44,12 @@ static cl::opt<bool> EnableLocalReassignment( | |||
| "may be compile time intensive"), | |||
| cl::init(false)); | |||
|
|
|||
| static cl::opt<float> MinWeightRatioNeededToEvictHint( | |||
| "min-weight-ratio-needed-to-evict-hint", cl::Hidden, | |||
| cl::desc("The minimum ration of weights needed in order for the live range with bigger weight to evict the other live range which" | |||
| "min-weight-ratio-needed-to-evict-hint", cl::Hidden, | ||
| cl::desc("The minimum ration of weights needed in order for the live range with bigger weight to evict the other live range which" | ||
| "satisfies a hint), | ||
| cl::init(7.5)); |
There was a problem hiding this comment.
This should default to the value that makes this change a NFC, so -inf?
| @@ -157,6 +163,10 @@ bool DefaultEvictionAdvisor::shouldEvict(const LiveInterval &A, bool IsHint, | |||
| return true; | |||
|
|
|||
| if (A.weight() > B.weight()) { | |||
| float WeightRatio = A.weight() / B.weight(); | |||
| "min-weight-ratio-needed-to-evict-hint", cl::Hidden, | ||
| cl::desc("The minimum ration of weights needed in order for the live range with bigger weight to evict the other live range which" | ||
| "satisfies a hint), | ||
| cl::init(7.5)); |
There was a problem hiding this comment.
To add on what @mtrofin said, could you explain the rationale for 7.5?
It feels random to me.
Added test |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
This patch makes a lot of difference |
mgudim
force-pushed
the
min-weight-ratio
branch
2 times, most recently
from
237091d to
f9fc97c
Compare
|
I've looked at the CI error - it is some assertion triggered from SLP vectorizer while compiling something in libcxx. I can't imagine how this change could trigger this, so I suspect something in the upstream is broken. |
Do not evict a live range if eviction will break existing hint without satisfying a new one and the ratio of weights is not big enough. The heuristic is disabled by default.
|
@mtrofin @qcolombet After rebasing, the CI passes. Is this good to merge? |
| if (AWeight > BWeight) { | ||
| float WeightRatio = BWeight == 0.0 ? std::numeric_limits<float>::infinity() | ||
| : AWeight / BWeight; | ||
| if (CanSplit && !IsHint && BreaksHint && |
There was a problem hiding this comment.
The CanSplit doesn't make much sense to me here.
Could you explain why this is desirable to check for this here?
| @@ -0,0 +1,76 @@ | |||
| # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5 | |||
| # RUN: llc %s -mtriple=riscv64 -run-pass=greedy,virtregrewriter \ | |||
| # RUN: -min-weight-ratio-needed-to-evict-hint=7.5 -o - | FileCheck %s | |||
There was a problem hiding this comment.
To go back to what I was saying wrt 7.5. I'm not questioning that this gives you the best outcome in what you measured, what I was asking is why it is.
Essentially what is the explanation that makes 7.5 a good number aside from "it fixes my problem".
Do not evict a live range if eviction will break existing hint without satisfying a new one and the ratio of weights is not big enough.