Skip to content
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 a header to simplex scheduler #7466

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

leothaud
Copy link
Contributor

@leothaud leothaud commented Aug 8, 2024

This pull request adds a header to the SimplexScheduler.cpp file, enabling the use of the simplex scheduler directly without the need to generate SSP modules in other projects that depend on CIRCT.

@leothaud leothaud changed the title split SimplexSchedulers into .h and .cpp [Scheduling] add a header to simplex scheduler Aug 8, 2024
@leothaud
Copy link
Contributor Author

leothaud commented Aug 8, 2024

Pinging potential reviewers: @jopperm

@jopperm
Copy link
Contributor

jopperm commented Aug 8, 2024

Thanks for the PR! At a first glance, I'm a bit lukewarm on making the simplex solver part of the API. Can you elaborate a bit on your use-case and the issues with SSP that you're seeing, please?

@jopperm jopperm self-requested a review August 8, 2024 13:24
@leothaud
Copy link
Contributor Author

leothaud commented Aug 8, 2024

I am working on a project in which I am adding a transformation that schedules many very similar problems. To improve efficiency, I considered modifying the Problem directly rather than modifying the SSP module and recreating a new Problem for each schedule.
Furthermore, integrating the scheduler into the API could allow custom problems and schedulers to be added. This doesn't seem possible at first glance, and it might be helpful in the project I'm working on.

@jopperm
Copy link
Contributor

jopperm commented Aug 13, 2024

I considered modifying the Problem directly rather than modifying the SSP module and recreating a new Problem for each schedule.

Yes, modifying the Problem is certainly the way to go. Do you need additional APIs there, e.g. to reset the solution properties or so?

In general, please note that I added the simplex-based solvers to have a very simple reference implementation in-tree with no external dependencies. If you care about efficiency, I would recommend using an actual (I)LP solver via OR-Tools. There has also been an unfinished effort to use the solver in MLIR's Presburger library (#4517) for scheduling, which could also be cool/worth dusting off.

@leothaud
Copy link
Contributor Author

leothaud commented Aug 13, 2024

Do you need additional APIs there, e.g. to reset the solution properties or so?

Scheduling an already scheduled problem overwrites the previous solution data, such as initiation interval or operations start times. Additional features may be helpful to work directly with the problem, such as enabling or disabling dependencies. This can be done by creating a subclass to the ChainingCyclicProblem class, which seems specific to my use case. Would it be useful to upstream a modifiable problem class?

If you care about efficiency, I would recommend using an actual (I)LP solver via OR-Tools.

As you mentioned, using an external solver would make scheduling more efficient but would add new dependencies to CIRCT. A common parent class for all schedulers would allow users to add their own external solver and make it extensible for new schedulers and new problems. Would you find adding a general scheduler interface and implementations to the API better than only adding the simplex scheduler?

@jopperm
Copy link
Contributor

jopperm commented Aug 14, 2024

Do you need additional APIs there, e.g. to reset the solution properties or so?

Scheduling an already scheduled problem overwrites the previous solution data, such as initiation interval or operations start times. Additional features may be helpful to work directly with the problem, such as enabling or disabling dependencies. This can be done by creating a subclass to the ChainingCyclicProblem class, which seems specific to my use case. Would it be useful to upstream a modifiable problem class?

I think I start to understand. Yes, if you want to selectively deactivate edges, I'd also say, model that as a new dependence property in a new sub-problem, but of course then you need to extend the scheduler as well...

On the other hand, I also don't see any fundamental issues with allowing problem components to be removed from the problem, and that sounds generally useful. It would still be the client's responsibility to call check()/verify() at appropriate times for validation. A design decision to be made would be how to handle properties of removed components -- do we keep them around, or use some form of callback to remove them when their associated operation/operator type/dependence is removed?

If you care about efficiency, I would recommend using an actual (I)LP solver via OR-Tools.

As you mentioned, using an external solver would make scheduling more efficient but would add new dependencies to CIRCT. A common parent class for all schedulers would allow users to add their own external solver and make it extensible for new schedulers and new problems. Would you find adding a general scheduler interface and implementations to the API better than only adding the simplex scheduler?

Interesting, could you outline what you have in mind, please?

My rationale for not exposing any interfaces on the scheduler side so far was that I didn't want to constrain the kinds of algorithms people can implement (like, everything is LP-based or so). In my mental model, the interface between client and scheduler are the different problem classes, and each scheduler implementation can opt into handling as many or as little problems as it wants. But of course, having more reusable building blocks for schedulers would be super useful as well.


One last thought: The simplex scheduler already has these hooks for additional constraints to be added by the subclasses. Would it help your use-case if the simplex schedulers would expose this functionality by accepting a lambda from the client?

@leothaud
Copy link
Contributor Author

leothaud commented Aug 19, 2024

I think I start to understand. Yes, if you want to selectively deactivate edges, I'd also say, model that as a new dependence property in a new sub-problem, but of course then you need to extend the scheduler as well...

On the other hand, I also don't see any fundamental issues with allowing problem components to be removed from the problem, and that sounds generally useful. It would still be the client's responsibility to call check()/verify() at appropriate times for validation. A design decision to be made would be how to handle properties of removed components -- do we keep them around, or use some form of callback to remove them when their associated operation/operator type/dependence is removed?

Indeed, we could add removeOperation(), removeDependence(), and removeOperatorType() methods in the Problem class. This would give more control over the Problem and could be helpful for custom passes that need scheduling. It should not cause trouble as long as check()/verify() are called. I don't know why we should keep them around if they are explicitly removed, but you may have a better global view of these practices than I do.

Interesting, could you outline what you have in mind, please?

My rationale for not exposing any interfaces on the scheduler side so far was that I didn't want to constrain the kinds of algorithms people can implement (like, everything is LP-based or so). In my mental model, the interface between client and scheduler are the different problem classes, and each scheduler implementation can opt into handling as many or as little problems as it wants. But of course, having more reusable building blocks for schedulers would be super useful as well.

I think exposing schedulers to the API is necessary to make it extensible and to make the Problem class extensible from outside of CIRCT, which may be useful. Exposing implemented schedulers will make it usable in other passes. If we don't want to constrain schedulers, we shouldn't make a virtual class that makes them need to schedule every Problem.
Maybe we could have a parametrized scheduler class that takes as a template the list of Problem classes it can schedule.

template <typename Derived, typename... Ps>
class Scheduler;

template <typename Derived, typename P, typename... Ps>
class Scheduler<Derived, P, Ps...> : public Scheduler<Derived, Ps...>, public Scheduler<Derived, P> {
};

template <typename Derived, typename P>
class Scheduler<Derived, P> {
public:
	virtual LogicalResult schedule(P problem) = 0;
};

template <typename Derived>
class Scheduler<Derived> {
public:
    virtual ~Scheduler() = default;
};

With a Scheduler class defined like this, a scheduler can schedule any number of problems, even if I do not know how to handle a scheduler that needs more parameters, like the LastOp for the SimplexScheduler.

One last thought: The simplex scheduler already has these hooks for additional constraints to be added by the subclasses. Would it help your use-case if the simplex schedulers would expose this functionality by accepting a lambda from the client?

For my use case, I think I won't modify the scheduler and won't need to add additional constraints for now. My current idea is to create a new Problem class that can disable some dependencies that won't be returned by the getDependences method. I think this will be efficient as I will have to schedule one problem with different small sets of disabled dependencies.

@jopperm
Copy link
Contributor

jopperm commented Aug 21, 2024

Indeed, we could add removeOperation(), removeDependence(), and removeOperatorType() methods in the Problem class. This would give more control over the Problem and could be helpful for custom passes that need scheduling. It should not cause trouble as long as check()/verify() are called. I don't know why we should keep them around if they are explicitly removed, but you may have a better global view of these practices than I do.

My concern was that these removeXXX() methods would need to be virtual and overridden in every subclass that introduces new properties, in order to remove the entries for the op/opr/dep from the corresponding maps. But I agree that this would be better than keeping properties dangling around in the internal maps.

@jopperm
Copy link
Contributor

jopperm commented Aug 21, 2024

[...] to make the Problem class extensible from outside of CIRCT, which may be useful.

Yes absolutely, OoT usability is super important. Let me pull in @7FM, who's also doing OoT problem modeling and scheduling -- do you have any pain points from your experience to add to this discussion?

With a Scheduler class defined like this [...]

TBH, I haven't fully understood the idea of the parameterized scheduler class, sorry. How would that help extensibility?

@leothaud
Copy link
Contributor Author

My concern was that these removeXXX() methods would need to be virtual and overridden in every subclass that introduces new properties, in order to remove the entries for the op/opr/dep from the corresponding maps. But I agree that this would be better than keeping properties dangling around in the internal maps.

Yes, this could totally be done, and it would be useful to remove incomingDelay and outgoingDelay in the ChainingProblem, for example.

TBH, I haven't fully understood the idea of the parameterized scheduler class, sorry. How would that help extensibility?

Sorry if the idea was not clear. I think using a parameterized scheduler class will allow schedulers to handle any combination of problems without being constrained to override the schedule function for every Problem as defined in the interface. For example, we would have

class ChainingCyclicSimplexScheduler : public Scheduler<ChainingCyclicSimplexScheduler, ChainingCyclicProblem> {
...
};

But we could imagine

class GenericScheduler: public Scheduler<GenericScheduler, Problem,
        CyclicProblem, ChainingProblem, SharedOperatorProblem,
        ModuloProblem, ChainingCyclicProblem> {
...
};

Furthermore, with this parameterization, it is possible to define schedulers for custom problems defined in an out-of-tree project.

class MyCustomScheduler: public Scheduler<MyCustomScheduler, MyCustomProblem>{
...
};

And, to define a function that uses a scheduler, it is not necessary to specify the scheduler. Instead, it could be taken as an argument and be instantiated with any compatible scheduler.

template <typename T>
void f(Scheduler<T, CyclicProblem> &scheduler) {
...
}

This was my idea to make it generic and extensible. I don't know if this is the best idea or the canonical way to do it, but I am open to any suggestion to improve it. This is a way to modify existing schedulers with a little cost and make them available in the API without making the header for SimplexScheduler available, which was my first idea.
Should I apply this idea to my fork to clarify and see if it works, or do you think I am going in the wrong direction?

@7FM
Copy link
Contributor

7FM commented Aug 22, 2024

do you have any pain points from your experience to add to this discussion?

My pain points so far have been the lack of an API to clone and modify problems. So yes removeXXX accessors would be greatly appreciated but I would also love something like a clone method. Currently, I have a handwritten copy function that tries to copy every property, which I tend to forget about when adding new properties / deriving from a new problem class. Another thing that I have manually implemented are methods to import and export solutions for my specific problem subclass, which is handy when you want to compute multiple solutions for the same problem instance without recreating it every time.

@jopperm
Copy link
Contributor

jopperm commented Aug 22, 2024

I've filed #7544, #7545, #7546 to track your proposals for the API extensions. I won't be able to work on this in the short term (traveling & preparing to move), so feel free to grab any of the issues.

Side note: I think the extensions all make sense, but also increase the amount of stuff a custom sub-problem needs to implement. I feel we're getting closer to the point where tablegen'ing the Problem classes could become a viable option... 😉

@jopperm
Copy link
Contributor

jopperm commented Aug 22, 2024

This was my idea to make it generic and extensible. I don't know if this is the best idea or the canonical way to do it, but I am open to any suggestion to improve it. This is a way to modify existing schedulers with a little cost and make them available in the API without making the header for SimplexScheduler available, which was my first idea.

  • I like the idea of a common super class for all schedulers, and the template-based approach that you propose seems nicer to model the opt-in than defining a free-function for each supported problem.
  • How would you handle passing additional arguments (objective function, cycle time, last op, etc.) to the concrete scheduler? Probably some form of argument forwarding could work.
  • Thinking about your dependence-deactivating use-case, how would your idea help you? Even if you can subclass the in-tree XYZSimplexScheduler, wouldn't its implementation still be opaque to you as you can only see/override the virtual LogicalResult schedule(P problem) method?

Should I apply this idea to my fork to clarify and see if it works [...]

Yeah would be great to see this in context.

@leothaud
Copy link
Contributor Author

How would you handle passing additional arguments (objective function, cycle time, last op, etc.) to the concrete scheduler? Probably some form of argument forwarding could work.

  • I don't see why the cycle time isn't a property of the problem. Isn't it tightly related to the value of incommingDelay, outgoingDelay & latency? For example, a 60ns multiplication would have a latency of 6 for a period of 10ns and a latency of 10 for a period of 6ns.
  • It seems that every scheduler takes the lastOp as an argument; we could add it as an argument for the schedule function of the interface. If the scheduler needs additional arguments, we will lose in generality. Should we expect the schedulers to work with common arguments and be free to add extra functions ? Or should the schedule function be variadic, and the types be checked at the beginning of the function?

Thinking about your dependence-deactivating use-case, how would your idea help you? Even if you can subclass the in-tree XYZSimplexScheduler, wouldn't its implementation still be opaque to you as you can only see/override the virtual LogicalResult schedule(P problem) method?

For my use case, my problem will be a subclass of the ChainingCyclicProblem class, and I should be able to use the ChainingCyclicSimplexScheduler.

@jopperm
Copy link
Contributor

jopperm commented Aug 26, 2024

  • I don't see why the cycle time isn't a property of the problem. Isn't it tightly related to the value of incommingDelay, outgoingDelay & latency?

It is, but when we defined the chaining problem, we didn't want to prescribe whether the cycle time should be a hard constraint (= making the problem infeasible if the cycle time cannot be achieved, feature of the problem), or rather its minimisation a possible objective (feature of the scheduler).

For example, a 60ns multiplication would have a latency of 6 for a period of 10ns and a latency of 10 for a period of 6ns.

That's an interesting line of thought, probably closer to reality than my mental model in which latency (abstract time steps) and physical delays are independent (well, the client needs to specify sensible values).

  • It seems that every scheduler takes the lastOp as an argument; we could add it as an argument for the schedule function of the interface. If the scheduler needs additional arguments, we will lose in generality. Should we expect the schedulers to work with common arguments and be free to add extra functions ? Or should the schedule function be variadic, and the types be checked at the beginning of the function?

For context, the reason most in-tree schedulers take the lastOp argument is that a) their objective is to minimise the start time of that operation (which is kind of the simplest possible objective) and b) it felt expensive to do graph analysis on the scheduling problem when usually the client already knows what the last op is (e.g. the terminator).

The benefit of the free function approach is complete freedom over how to specify the objective function and any additional user constraints (e.g. some maximum latency, or treat the cycle time as a hard constraint, etc.). I didn't really have a good idea in the past how to generalise/abstract these, so I left it scheduler-specific, but I'd be open to change the interface to a "common case + escape hatch" system.

@leothaud
Copy link
Contributor Author

leothaud commented Sep 1, 2024

I added a Scheduler interface and adapted the existing schedulers and the documentation. If the pull request is accepted, squashing the commits may be a good idea to have a better git log, but for now, I left it like this to get a better modifications history.

  • The Scheduler interface uses a template to declare the Problem types that are handled. I added a static_assert to check that the template elements are subclasses of Problem and another static_assert to check that the list is not empty. I think a scheduler that cannot schedule anything makes little sense.

  • I transferred the cycle-time into the ChainingProblem because, for me, it is linked with the proportion of incoming/outgoing delays and latency. I renamed it targetCycleTime because you mentioned that it may be a hard limit.

I think adding scheduling to the API using a Scheduler interface is the correct way. I am open to any review of my interface. Do you think we could add schedulers to the API using something like that?

@jopperm
Copy link
Contributor

jopperm commented Sep 5, 2024

Thank you for putting this together! I would like to discuss and break down the changes into smaller parts (this is too big for a single PR anyways).

The scheduler base class is kind of nice, but I still don't like the idea of prescribing a fixed interface for the schedule method. The simplex scheduler is the only implementation that maintains state, and the only one we have a use-case (yours) to extend right now. I'm ok with breaking with the current convention that schedulers only define free-function entry-points, but I'm not convinced yet that we should/need to change all reference schedulers at once. So I'd say let's go back to making the simplex scheduler extensible OoT for now, as you originally proposed. Sorry for the detour.

Would we then go back to just moving the current scheduler class declarations to a header, or do you actually need the scheduler to be templated? The current implementation is not perfect (looking at you, getProblem() method), but the sheer number of using declarations like in the your proposed ModuloSimplexScheduler is also a bit inelegant IMHO 😉

In the end, I believe the extension hooks that we can reasonably support for subclasses are the ones we have now, corresponding to the virtual methods on SimplexSchedulerBase: a) adding constraints (fillConstraintRow(...)) and b) driving the solution process (schedule()) -- please propose changes or extensions to those, if needed.

Lastly, if you still need to move the cycle time into the problem, make it an InstanceProperty (alias for std::optional), please. If and only if it is set, a feasible solution must obey it. This way we can support both the current and the new behavior. This should then be a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants