-
Notifications
You must be signed in to change notification settings - Fork 298
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
[Ibis] Add scheduling prototype #6239
Conversation
1. During prepare scheduling, we also assign `ssp.operator_type` attributes to the operations. The expected operator symbol names are equal to the operation names. 2. Add `ibis-add-operator-library` which injects an `ssp.library` operation at the top-level module with some default valued operators 3. From there, we can simply run `pipeline-schedule-linear` on the `sblock`s.
opLib.setSymNameAttr(b.getStringAttr(kIbisOperatorLibName)); | ||
b.setInsertionPointToStart(opLib.getBodyBlock()); | ||
|
||
// Provide definitions for some comb ops - just latency properties for now. |
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.
We use truth table a lot.
I pretty distinctly remember when the SSP dialect was added, it was supposed to be used "only for testing". The right way to do this was going to be with an API. |
// Provide definitions for some comb ops - just latency properties for now. | ||
|
||
// Arithmetic operators | ||
addOperator<comb::AddOp>(b, 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.
The problem with static cost model is that 3-input adds have higher cost then 2 (or 1). Cost functions need to account for that. (I don't know if that's supported by the API, though IIRC it is. Or I at least commented that it should be in a PR years ago.)
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 suppose one could write out operator types for all the different number of inputs detected. But that gets ugly.
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.
fully agree - there are many things in this model (ssp library op/the existing pipeline scheduling pass) that obviously doesn't scale nor will produce good QoR for "real" circuits.
|
||
// Attach the operator lib attribute | ||
pipeline->setAttr( | ||
"operator_lib", |
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 should really be a dialect attribute in SSP.
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.
From SSP dialect rationale: "The SSP dialect’s main use-cases are testing, benchmarking and rapid-prototyping. It is strictly a companion to the existing scheduling infrastructure, and clients (HLS flows etc.) are advised to use the C++ APIs directly."
So if this really is "just a prototype" then this is fine. Please add a TODO in the code to replace the use of SSP with the C++ API when it grows out of the prototype phase.
This approach has been taken to do the minimum to hook up our pipelines to the existing pipeline scheduling passes - which rely on this style of embedded |
ssp.operator_type
attributes to the operations. The expected operator symbol names are equal to the operation names.ibis-add-operator-library
which injects anssp.library
operation at the top-level module with some default valued operators.pipeline-schedule-linear
on thesblock
s.... and snuck in a change of the
high_level.mlir
test to not usescf
; the main issue here being theindex
-typed values and the lack of an index "materialization" pass.