-
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
[Pipeline] Add "ext" inputs to pipelines #5347
Conversation
f9d6663
to
b1d1679
Compare
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!
|
||
// The entry block defines a lot of SSA values... | ||
// drop everything but the 'input's to the stage | ||
return stage->getArguments().take_front(getInputs().size()); |
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 am uncomfortable with making the assumption that the inputs are the first group of block args, but I don't think there's any alternative. Matt was commenting on something similar (having to rely on port numbers rather than named symbols). You might think on a solution for both and if the problems relate.
Just something I want to put in your brain.
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.
For this, is there anything that ODS generates which could be used to get the offset? (Which would be zero with the current ODS def.)
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.
Changed to
return stage->getArguments().slice(getInputs().getBeginOperandIndex(), getInputs().size());
inputs = mod.getArguments().take_front(nStageInputArgs); | ||
extInputs = mod.getArguments().slice( | ||
nStageInputArgs, parent.getStageExtInputs(block).size()); | ||
clock = mod.getArgument(mod.getNumArguments() - 2); |
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.
Magic constants. Eww. Can you define some (perhaps public) offset constants for these? Or an attribute? We should really come up with an efficient way to access port indexes by name.
7cc3cb6
to
16712d3
Compare
`ext` inputs are inputs which are accessible in any pipeline stage, and which will never be registered during register materialization. If you imagine a pipeline as signals going from left to right with registers in between, `ext` inputs are inputs that hook in from the top or bottom, into any pipeline stage. In hardware (outlined) lowering, the external inputs are only provided to stages which actually reference them. **Note**: The pipeline op signature is getting unruly, given that all inputs are bundled into the single `^bb0` definition. In a future commit, i plan to specialize the printer/parser for the pipeline op to mimick something like the `scf` ops, such that we have a bit more sane format. ```mlir %out:2 = pipeline.scheduled(%arg0) ext(%arg1 : i32) clock %clk reset %rst : (i32, i32) -> (i32, i32) { ^bb0(%a0 : i32, %ext0: i32): // vs %out:2 = pipeline.scheduled(%a0 : i32 = %arg0) ext(%ext0 = %arg1 : i32) clock %clk reset %rst -> (i32, i32) { ```
41dc5e6
to
9bd8741
Compare
ext
inputs are inputs which are accessible in any pipeline stage, and which will never be registered during register materialization. If you imagine a pipeline as signals going from left to right with registers in between,ext
inputs are inputs that hook in from the top or bottom, into any pipeline stage.In hardware (outlined) lowering, the external inputs are only provided to stages which actually reference them:
Note: The pipeline op signature is getting unruly, given that all inputs are bundled into the single
^bb0
definition. In a future commit, i plan to specialize the printer/parser for the pipeline op to mimick something like thescf
ops, such that we have a bit more sane format.