ConstraintElim: add dry-run routine to fail early #99670
Conversation
Add a dry-run routine that computes a conservative estimate of the number of rows and columns that the transform will require, and fail early if the estimates exceed the upper bounds. This patch has a small overhead, but improves compile-time on one benchmark significantly. The overhead will be compensated for in a follow-up patch, where ConstraintSystem is ported to use a Matrix data structure, performing the full allocation ahead-of-time using these estimates.
|
@llvm/pr-subscribers-llvm-transforms Author: Ramkumar Ramachandra (artagnon) ChangesAdd a dry-run routine that computes a conservative estimate of the number of rows and columns that the transform will require, and fail early if the estimates exceed the upper bounds. This patch has a small overhead, but improves compile-time on one benchmark significantly. -- 8< -- Full diff: https://github.com/llvm/llvm-project/pull/99670.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
index c31173879af1e..369a6daa4d970 100644
--- a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
@@ -40,7 +40,6 @@
#include "llvm/Transforms/Utils/Cloning.h"
#include "llvm/Transforms/Utils/ValueMapper.h"
-#include <cmath>
#include <optional>
#include <string>
@@ -57,6 +56,10 @@ static cl::opt<unsigned>
MaxRows("constraint-elimination-max-rows", cl::init(500), cl::Hidden,
cl::desc("Maximum number of rows to keep in constraint system"));
+static cl::opt<unsigned> MaxColumns(
+ "constraint-elimination-max-cols", cl::init(50), cl::Hidden,
+ cl::desc("Maximum number of columns to keep in constraint system"));
+
static cl::opt<bool> DumpReproducers(
"constraint-elimination-dump-reproducers", cl::init(false), cl::Hidden,
cl::desc("Dump IR to reproduce successful transformations."));
@@ -303,6 +306,7 @@ class ConstraintInfo {
void popLastNVariables(bool Signed, unsigned N) {
getCS(Signed).popLastNVariables(N);
}
+ const DataLayout &getDataLayout() const { return DL; }
bool doesHold(CmpInst::Predicate Pred, Value *A, Value *B) const;
@@ -1491,7 +1495,7 @@ removeEntryFromStack(const StackEntry &E, ConstraintInfo &Info,
/// Check if either the first condition of an AND or OR is implied by the
/// (negated in case of OR) second condition or vice versa.
static bool checkOrAndOpImpliedByOther(
- FactOrCheck &CB, ConstraintInfo &Info, Module *ReproducerModule,
+ const FactOrCheck &CB, ConstraintInfo &Info, Module *ReproducerModule,
SmallVectorImpl<ReproducerEntry> &ReproducerCondStack,
SmallVectorImpl<StackEntry> &DFSInStack) {
@@ -1671,18 +1675,91 @@ tryToSimplifyOverflowMath(IntrinsicInst *II, ConstraintInfo &Info,
return Changed;
}
-static bool eliminateConstraints(Function &F, DominatorTree &DT, LoopInfo &LI,
- ScalarEvolution &SE,
- OptimizationRemarkEmitter &ORE) {
- bool Changed = false;
+/// Performs a dry run of AddFact, computing a conservative estimate of the
+/// number of new variables introduced.
+static void dryRunAddFact(CmpInst::Predicate Pred, Value *A, Value *B,
+ const ConstraintInfo &Info, unsigned &EstimatedRowsA,
+ unsigned &EstimatedRowsB,
+ unsigned &EstimatedColumns) {
+ auto UpdateEstimate = [&Info, &EstimatedRowsA, &EstimatedRowsB,
+ &EstimatedColumns](CmpInst::Predicate Pred, Value *A,
+ Value *B) {
+ SmallVector<Value *> NewVars;
+ auto R = Info.getConstraint(Pred, A, B, NewVars);
+
+ // We offset it by 1 due to logic in addFact.
+ unsigned NewEstimate =
+ count_if(R.Coefficients, [](int64_t C) { return C != 0; }) + 1;
+
+ EstimatedColumns = std::max(EstimatedColumns, NewEstimate);
+ if (R.IsSigned)
+ ++EstimatedRowsA;
+ else
+ ++EstimatedRowsB;
+ };
+
+ UpdateEstimate(Pred, A, B);
+
+ // What follows is a dry-run of transferToOtherSystem.
+ auto IsKnownNonNegative = [&Info](Value *V) {
+ return Info.doesHold(CmpInst::ICMP_SGE, V,
+ ConstantInt::get(V->getType(), 0)) ||
+ isKnownNonNegative(V, Info.getDataLayout(),
+ MaxAnalysisRecursionDepth - 1);
+ };
+
+ if (!A->getType()->isIntegerTy())
+ return;
+
+ switch (Pred) {
+ default:
+ break;
+ case CmpInst::ICMP_ULT:
+ case CmpInst::ICMP_ULE:
+ if (IsKnownNonNegative(B)) {
+ UpdateEstimate(CmpInst::ICMP_SGE, A, ConstantInt::get(B->getType(), 0));
+ UpdateEstimate(CmpInst::getSignedPredicate(Pred), A, B);
+ }
+ break;
+ case CmpInst::ICMP_UGE:
+ case CmpInst::ICMP_UGT:
+ if (IsKnownNonNegative(A)) {
+ UpdateEstimate(CmpInst::ICMP_SGE, B, ConstantInt::get(B->getType(), 0));
+ UpdateEstimate(CmpInst::getSignedPredicate(Pred), A, B);
+ }
+ break;
+ case CmpInst::ICMP_SLT:
+ if (IsKnownNonNegative(A))
+ UpdateEstimate(CmpInst::ICMP_ULT, A, B);
+ break;
+ case CmpInst::ICMP_SGT:
+ if (Info.doesHold(CmpInst::ICMP_SGE, B, ConstantInt::get(B->getType(), -1)))
+ UpdateEstimate(CmpInst::ICMP_UGE, A, ConstantInt::get(B->getType(), 0));
+ if (IsKnownNonNegative(B))
+ UpdateEstimate(CmpInst::ICMP_UGT, A, B);
+ break;
+ case CmpInst::ICMP_SGE:
+ if (IsKnownNonNegative(B))
+ UpdateEstimate(CmpInst::ICMP_UGE, A, B);
+ break;
+ }
+}
+
+/// Performs a dry run of the transform, computing a conservative estimate of
+/// the total number of columns we need in the underlying storage.
+static std::tuple<State, unsigned, unsigned>
+dryRun(Function &F, DominatorTree &DT, LoopInfo &LI, ScalarEvolution &SE) {
DT.updateDFSNumbers();
SmallVector<Value *> FunctionArgs;
for (Value &Arg : F.args())
FunctionArgs.push_back(&Arg);
- ConstraintInfo Info(F.getDataLayout(), FunctionArgs);
State S(DT, LI, SE);
- std::unique_ptr<Module> ReproducerModule(
- DumpReproducers ? new Module(F.getName(), F.getContext()) : nullptr);
+ unsigned EstimatedColumns = FunctionArgs.size() + 1;
+
+ // EstimatedRowsA corresponds to SignedCS, and EstimatedRowsB corresponds to
+ // UnsignedCS.
+ unsigned EstimatedRowsA = 0, EstimatedRowsB = 1;
+ ConstraintInfo Info(F.getDataLayout(), FunctionArgs);
// First, collect conditions implied by branches and blocks with their
// Dominator DFS in and out numbers.
@@ -1725,12 +1802,90 @@ static bool eliminateConstraints(Function &F, DominatorTree &DT, LoopInfo &LI,
return A.NumIn < B.NumIn;
});
+ for (const FactOrCheck &CB : S.WorkList) {
+ ICmpInst::Predicate Pred;
+ Value *A, *B;
+ if (CB.isCheck()) {
+ // What follows is a dry-run of checkOrAndOpImpliedByOther, without
+ // assuming that instructions have been simplified, as they would have
+ // during the course of normal operation.
+ auto *ContextInst = CB.getContextInst();
+ if (auto *Cmp =
+ dyn_cast_or_null<ICmpInst>(CB.getInstructionToSimplify())) {
+ unsigned OtherOpIdx = ContextInst->getOperand(0) == Cmp ? 1 : 0;
+ if (match(ContextInst, m_LogicalOp()) &&
+ match(ContextInst->getOperand(OtherOpIdx),
+ m_ICmp(Pred, m_Value(A), m_Value(B)))) {
+ if (match(ContextInst, m_LogicalOr()))
+ Pred = CmpInst::getInversePredicate(Pred);
+ dryRunAddFact(Pred, A, B, Info, EstimatedRowsA, EstimatedRowsB,
+ EstimatedColumns);
+ }
+ }
+ continue;
+ }
+ if (!CB.isConditionFact()) {
+ Value *X;
+ if (match(CB.Inst, m_Intrinsic<Intrinsic::abs>(m_Value(X)))) {
+ if (cast<ConstantInt>(CB.Inst->getOperand(1))->isOne())
+ dryRunAddFact(CmpInst::ICMP_SGE, CB.Inst,
+ ConstantInt::get(CB.Inst->getType(), 0), Info,
+ EstimatedRowsA, EstimatedRowsB, EstimatedColumns);
+ dryRunAddFact(CmpInst::ICMP_SGE, CB.Inst, X, Info, EstimatedRowsA,
+ EstimatedRowsB, EstimatedColumns);
+ continue;
+ }
+
+ if (auto *MinMax = dyn_cast<MinMaxIntrinsic>(CB.Inst)) {
+ Pred = ICmpInst::getNonStrictPredicate(MinMax->getPredicate());
+ dryRunAddFact(Pred, MinMax, MinMax->getLHS(), Info, EstimatedRowsA,
+ EstimatedRowsB, EstimatedColumns);
+ dryRunAddFact(Pred, MinMax, MinMax->getRHS(), Info, EstimatedRowsA,
+ EstimatedRowsB, EstimatedColumns);
+ continue;
+ }
+ }
+
+ if (CB.isConditionFact()) {
+ Pred = CB.Cond.Pred;
+ A = CB.Cond.Op0;
+ B = CB.Cond.Op1;
+ } else {
+ bool Matched = match(CB.Inst, m_Intrinsic<Intrinsic::assume>(
+ m_ICmp(Pred, m_Value(A), m_Value(B))));
+ (void)Matched;
+ assert(Matched && "Must have an assume intrinsic with a icmp operand");
+ }
+ dryRunAddFact(Pred, A, B, Info, EstimatedRowsA, EstimatedRowsB,
+ EstimatedColumns);
+ }
+ return {S, std::max(EstimatedRowsA, EstimatedRowsB), EstimatedColumns};
+}
+
+static bool eliminateConstraints(Function &F, DominatorTree &DT, LoopInfo &LI,
+ ScalarEvolution &SE,
+ OptimizationRemarkEmitter &ORE) {
+ bool Changed = false;
+ const auto &[S, EstimatedRows, EstimatedColumns] = dryRun(F, DT, LI, SE);
+
+ // Fail early if estimates exceed limits. Row estimate could be off by up to
+ // 40%.
+ if (EstimatedRows > 1.4 * MaxRows || EstimatedColumns > MaxColumns)
+ return false;
+
+ SmallVector<Value *> FunctionArgs;
+ for (Value &Arg : F.args())
+ FunctionArgs.push_back(&Arg);
+ ConstraintInfo Info(F.getDataLayout(), FunctionArgs);
+ std::unique_ptr<Module> ReproducerModule(
+ DumpReproducers ? new Module(F.getName(), F.getContext()) : nullptr);
+
SmallVector<Instruction *> ToRemove;
// Finally, process ordered worklist and eliminate implied conditions.
SmallVector<StackEntry, 16> DFSInStack;
SmallVector<ReproducerEntry> ReproducerCondStack;
- for (FactOrCheck &CB : S.WorkList) {
+ for (const FactOrCheck &CB : S.WorkList) {
// First, pop entries from the stack that are out-of-scope for CB. Remove
// the corresponding entry from the constraint system.
while (!DFSInStack.empty()) {
diff --git a/llvm/test/Transforms/ConstraintElimination/max-row-limit.ll b/llvm/test/Transforms/ConstraintElimination/max-row-column-limit.ll
similarity index 81%
rename from llvm/test/Transforms/ConstraintElimination/max-row-limit.ll
rename to llvm/test/Transforms/ConstraintElimination/max-row-column-limit.ll
index 0e078109ed663..2f3b62dc5dab7 100644
--- a/llvm/test/Transforms/ConstraintElimination/max-row-limit.ll
+++ b/llvm/test/Transforms/ConstraintElimination/max-row-column-limit.ll
@@ -1,7 +1,9 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt -passes=constraint-elimination -S %s | FileCheck --check-prefixes=COMMON,SIMP %s
-; RUN: opt -passes=constraint-elimination -constraint-elimination-max-rows=9 -S %s | FileCheck --check-prefixes=COMMON,SIMP %s
-; RUN: opt -passes=constraint-elimination -constraint-elimination-max-rows=8 -S %s | FileCheck --check-prefixes=COMMON,NOSIMP %s
+; RUN: opt -passes=constraint-elimination -S %s | FileCheck --check-prefixes=SIMP %s
+; RUN: opt -passes=constraint-elimination -constraint-elimination-max-rows=8 -S %s | FileCheck --check-prefixes=SIMP %s
+; RUN: opt -passes=constraint-elimination -constraint-elimination-max-cols=6 -S %s | FileCheck --check-prefixes=SIMP %s
+; RUN: opt -passes=constraint-elimination -constraint-elimination-max-rows=7 -S %s | FileCheck --check-prefixes=NOSIMP %s
+; RUN: opt -passes=constraint-elimination -constraint-elimination-max-cols=5 -S %s | FileCheck --check-prefixes=NOSIMP %s
define i1 @test_max_row_limit(i32 %l0, i32 %l1, i32 %l2, i32 %l3, i32 %l4) {
@@ -22,7 +24,8 @@ define i1 @test_max_row_limit(i32 %l0, i32 %l1, i32 %l2, i32 %l3, i32 %l4) {
; SIMP-NEXT: [[C4:%.*]] = icmp uge i32 [[L4:%.*]], 100
; SIMP-NEXT: br i1 [[C4]], label [[BB5:%.*]], label [[EXIT]]
; SIMP: bb5:
-; SIMP-NEXT: ret i1 true
+; SIMP-NEXT: [[C5:%.*]] = icmp sge i32 [[L4:%.*]], 100
+; SIMP-NEXT: ret i1 [[C5]]
; SIMP: exit:
; SIMP-NEXT: ret i1 false
;
@@ -43,7 +46,7 @@ define i1 @test_max_row_limit(i32 %l0, i32 %l1, i32 %l2, i32 %l3, i32 %l4) {
; NOSIMP-NEXT: [[C4:%.*]] = icmp uge i32 [[L4:%.*]], 100
; NOSIMP-NEXT: br i1 [[C4]], label [[BB5:%.*]], label [[EXIT]]
; NOSIMP: bb5:
-; NOSIMP-NEXT: [[C5:%.*]] = icmp uge i32 [[L4]], 100
+; NOSIMP-NEXT: [[C5:%.*]] = icmp sge i32 [[L4]], 100
; NOSIMP-NEXT: ret i1 [[C5]]
; NOSIMP: exit:
; NOSIMP-NEXT: ret i1 false
@@ -69,11 +72,9 @@ bb4:
br i1 %c4, label %bb5, label %exit
bb5:
- %c5 = icmp uge i32 %l4, 100
+ %c5 = icmp sge i32 %l4, 100
ret i1 %c5
exit:
ret i1 false
}
-;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
-; COMMON: {{.*}}
|
|
Compile-time impact on my dataset: |
nikic
left a comment
nikic
left a comment
There was a problem hiding this comment.
This does not look particularly compelling to me. It's a regression on average, and also duplicates a lot of logic.
Perhaps it is possible to strip down the complicated logic, obtaining a cheaper estimate, and reducing the logic-duplication. I will benchmark it on the top regressions, and see what we can strip out. |
artagnon
force-pushed
the
perf/constraintelim-dryrun
branch
from
7d91373 to
50c2e95
Compare
|
New results look good: http://llvm-compile-time-tracker.com/compare.php?from=c5ee3c05ca61f3fae11337c5853aee7b450a9dc6&to=50c2e95b9a66395d5f6b62a99e67fec1a874f8f5&stat=instructions%3Au. @dtcxzyw Could you kindly check the latest commit against your data set? |
|
artagnon
force-pushed
the
perf/constraintelim-dryrun
branch
from
a1c6274 to
da823b7
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
artagnon
force-pushed
the
perf/constraintelim-dryrun
branch
from
da823b7 to
11b306f
Compare
|
I've made one more optimization, which, according to Compile Time Tracker, is a small optimization: http://llvm-compile-time-tracker.com/compare.php?from=c5ee3c05ca61f3fae11337c5853aee7b450a9dc6&to=11b306fae6c365a152d8fb9b61846ca733ac87c1&stat=instructions%3Au. I'm running out of ideas to optimize the dry-run routine further. @dtcxzyw Could you kindly re-check with the latest commit? |
Top 5 improvements: |
|
Okay, so it's a near-neutral change overall, with huge improvements in a some cases. I don't think it's possible to optimize this further, without duplicating decompose(), and that is probably undesirable. Although the benefits of this patch on its own are questionable, it is a prerequisite for another neutral change: the change to the Matrix data structure. The change to the Matrix data structure is a prerequisite for building more complicated things on top of ConsttraintSystem: I'm planning to write a solver-based dependence analysis. |
artagnon
force-pushed
the
perf/constraintelim-dryrun
branch
from
9537b21 to
f0d674c
Compare
|
Managed to eek out another small optimization: http://llvm-compile-time-tracker.com/compare.php?from=c5ee3c05ca61f3fae11337c5853aee7b450a9dc6&to=f0d674c9f5aa601c0e5472f208ff96f3d855d9b6&stat=instructions%3Au. @dtcxzyw Could you kindly re-check against the latest commit? |
artagnon
force-pushed
the
perf/constraintelim-dryrun
branch
from
76831e7 to
f0d674c
Compare
I also wonder if this suffices to establish whether the early conservative estimation of rows/columns outweighs the somewhat noticeable code duplication, maybe worth it to run SPEC with this before further reviewing? |
Certainly. Unfortunately, I don't have access to a machine with SPEC set up. Perhaps someone else has access to one? I think it might be possible to get rid of at least some of the code duplication using a template parameter. Also, I think the regressions are coming from tiny compilation units, for which I think we can short-circuit the dry-run using a rough estimate like number of items in the worklist. |
I can check that this is NFC from a code-gen perspective for SPEC and some other larger workloads in the next few days. Please ping me if you don't hear from me by Thursday :) |
Ping :) |
|
I managed to get access to a copy of SPEC2006. I'm happy to report that there are no binary changes, and that the compile time has improved by 0.9%, as measured with perf. |
|
I had a chance to check SPEC2017 and there are a few cases where fewer conditions are simplified (with -O3) In Sqlite from the test suite, there also are a few cases where we have fewer simplifications (but no binary changes probably due to them being simplified later). I am wondering if we should aim for a check like below, to surface more cases where the behavior changes (with that change, I also tried bootstrapping Clang but couldn't get very far) |
|
Presumably some of those issues would go away with a larger default for MaxColumns? |
My gut reaction is that this diff will not be helpful: we will get a lot of asserts in the case of binary changes, and these asserts will not have information to meaningfully update the estimates. The dry-run estimation can't be used for a logically precise condition: it is a cheap estimation that "works in practice". I wish we had some kind of formula for the upper-bound we could just lift out from a paper (as is the case for many algorithms like FM), but this is not possible for ConstraintElimination.
My intuition from writing the dry-run estimation routine is that the 1.4 factor is the more problematic issue, and not MaxColumns. We should bump the factor to 1.6, and try again: the reason I used 1.4 is because I wanted a fine upper bound so we don't lose the benefits of the patch; 1.6 could work just as well, but a factor that's too high might not yield such great overall results. I think the row-estimation is off by a constant factor (modulo minor error), so multiplying a constant factor should be possible. I've observed that the column-estimation is pretty accurate, although it is possible that MaxColumns would need to be bumped up to 64 if the row-estimation times the constant factor is correct. |
Hm I think we should aim for a path forward without missing any simplification cases if possible. It also changes the behavior of the max column limit IIRC, as previously we just skipped in the case of hitting the limit until we removed rows again, then we continue. The current patch means we don't even try to simplify anything if the max is going to be above the threshold, which doesn't seem ideal. Thinking also in the context of going forward with the new matrix data structure, some other alternatives that come to mind
I don't think we need to aim for the current patch having a positive compile-time impact on its own if it comes at the cost of lost simplifications; having a positive impact in combination with the new matrix data type should be sufficient IMO. |
I agree with your reasoning fully, since it is impossible to know if there will be a case where fewer simplifications are made. Even if we manage to get SPEC 2017 to not have binary changes, there could be degenerate cases out there that will miss some simplifications because of the dry-run-cutoff. There are, however, fine trade-offs we need to make, which I will explain shortly.
Okay, so the current patch is a strict over-estimate. It doesn't under-estimate under any circumstance. The only reason for MaxColumns to exist is for the test.
The crux of the issue here is that the dry-run estimation adds a very small overhead, and the Matrix data structure is just a zero-cost abstraction (such a small benefit, that the benchmarks classify it as noise). We would therefore see a small negative impact overall after applying both patches. Although I'm not sure, I think removing the dry-run routine, and column-resizing the Matrix at runtime would make it a non-zero-cost abstraction. The reason I developed the Matrix data structure is so that we can take slices of the Matrix, and operate on that slice, as if it were first-class. This would allow us to parse dependency information into a larger Matrix, and pass a sub-Matrix to the solver: dependency information has a "domain" and "range" which are parsed into separate column ranges, but solvers cannot operate on "mappings"; you need to extract a "set" out of the mapping, and operate on that. The way Presburger solves this problem is by copying pieces of "mappings" into "sets", which is far from ideal. Another benefit is that it would allow us to experiment with implementing different ILP algorithms like Simplex-based methods, which are supposedly faster (although we'd have to experiment to find out if it really is faster in our case). Although it is technically possible to use vector-of-vectors to implement Simplex-based methods, the most natural data structure is a "tableau" (or Matrix). Now, it wouldn't be ideal to land a patch that has negative impact overall, with the promise that it could be addressed in the future. The current patch is in its current form because that was the trade-off calculation I performed in my head. |
|
Tweaking the patch was pretty straight-forward actually. There are no binary changes on SPEC2017 after tweaking the factor to 1.6. Compile time improvement is 0.6%. As I mentioned, there is no way to absolutely guarantee that there won't be binary changes in a theoretical degenerate case, but I think the trade-off I've chosen is practical, and will allow us to move forward. |
|
So, I had some more time to think about this, and there is one other slightly sub-optimal way forward: I experimented with dropping the dry-run routine entirely, and hard-coding 50 columns in the Matrix. If we feel that the small compile-time regression caused by this is acceptable, and that the dry-run routine is unacceptable, this would be one potential way forward: http://llvm-compile-time-tracker.com/compare.php?from=76e2307e695330912b89e51f65f75ca0f4c8bd24&to=1be3ca30a469ff4b2a45a2eb5bba3db230afe95e&stat=instructions:u. Otherwise, I can also experiment with column-resizing the Matrix dynamically, although I'm less optimistic about that. |
6 participants
Add a dry-run routine that computes a conservative estimate of the number of rows and columns that the transform will require, and fail early if the estimates exceed the upper bounds. This patch has almost no overhead, and improves compile-time on some benchmarks significantly.
-- 8< --
Results from LLVM Compile Time Tracker: http://llvm-compile-time-tracker.com/compare.php?from=c5ee3c05ca61f3fae11337c5853aee7b450a9dc6&to=50c2e95b9a66395d5f6b62a99e67fec1a874f8f5&stat=instructions%3Au.