-
Notifications
You must be signed in to change notification settings - Fork 285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Scheduling] Add Presburger Simplex scheduler #4517
base: main
Are you sure you want to change the base?
Conversation
unsigned latency = *prob.getLatency(*prob.getLinkedOperatorType(src)); | ||
row.back() = -latency; // note the negation | ||
if (src != | ||
dst) { // note that these coefficients just zero out in self-arcs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This formatting seems broken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Groverkss, this is excellent work that just needs a minor revision. I have a few nits and a clarification request on the approach itself. Also, it looks like you're missing a dependency on the Presburger library in the CMakeList.txt
.
class Solver : private LexSimplex { | ||
public: | ||
Solver(Problem &prob, unsigned numObj) | ||
: LexSimplex(1 + numObj + prob.getOperations().size()), prob(prob) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain the 1+
in a comment.
/// Create a latency constraint representing the given dependence. | ||
/// The constraint is represented as: | ||
/// dstOpStartTime >= srcOpStartTime + latency. | ||
void createLatencyConstraint(MutableArrayRef<MPInt> row, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd call this precedence constraint.
/// Create a cyclic latency constraint representing the given dependence and | ||
/// the given dependence distance. | ||
/// The constraint is represented as: | ||
/// dstOpStartTime >= srcOpStartTime + latency + II * distance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be minus II * distance, coming from:
srcOpStartTime + latency <= dstOpStartTime + distance * II
Problem::Dependence dep, | ||
const MPInt &distance) { | ||
createLatencyConstraint(row, dep); | ||
row[0] = distance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please introduce a constant for the variable representing the II.
// There is a single objective, minimize the last operation. | ||
{ | ||
SmallVector<MPInt, 8> row(solver.getNumCols()); | ||
row[1] = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, please introduce a constant for the magic number.
} | ||
} | ||
|
||
// The constraints from dependence are built in a way that the solution always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dependences
|
||
for (auto *op : prob.getOperations()) | ||
prob.setStartTime(op, | ||
int64_t(sample[solver.getOpIndex(op)].getAsInteger())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start times are stored as unsigned
in the Problem
, so I think it would be clearer to cast directly to unsigned
here instead of int64_t
.
} | ||
|
||
// The constraints from dependence are built in a way that the solution always | ||
// has integer rational start times. So, we can solve rationally keeping II |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the reference to II a copy'n'paste artifact?
|
||
ArrayRef<Fraction> res = *sample; | ||
|
||
// If we have an integer II, we can just return the solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This intuitively makes sense to me, but can you provide a reference to a paper or text book for the underlying theoretical reasoning please?
// is valid, this is a valid solution. | ||
MaybeOptimum<SmallVector<Fraction, 8>> solveRationally() { | ||
MaybeOptimum<SmallVector<Fraction, 8>> sample = | ||
LexSimplex::findRationalLexMin(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The II is not part of the objective, right? So the solver may theoretically return any value here, because the II is just a normal decision variable if I understand correctly.
I would have expected that you optimize for the smallest II first, ceil/fix it, and then optimize for the start time of the last operation.
@Groverkss I transitioned (#4523) the scheduling test cases to use the SSP dialect and the |
Status? |
This patch adds a new scheduler for Scheduling problems. This scheduler utilizes the Presburger library's Simplex to find the optimal solution.
The Presburger scheduler lives in-tree in MLIR, which means there are no external dependencies and allows using arbitrary precision.