-
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] Refactor pipeline dialect to be block-based #5332
Conversation
This commit refactors the pipeline dialect to be block-based. This brings a major representational change in the form of: 1. The pipeline is no longer defined by a lexical ordering of operations and insertion of `pipeline.stagesep` operations to separate stages. Instead, pipeline stages are defined by blocks. 2. Control flow between blocks are defined by `pipeline.stage` operations. 3. Like in the current version, the pipeline can exist in register dematerialized and materialized forms. In the dematerialized form, stages (`Block`s) have no arguments. In the materialized form, stages have arguments. 4. It is the `pipeline.stage` operations which infers whether to register a value or pass it directly (i.e. a wire) to the next stage. The motivation for this change is to improve the hierarchy of the IR, instead of relying on lexical ordering. This change also allows for more natural traversal of stages (`Block`s), as well as dataflow analysis of the pipeline, which now is analogous to control flow analysis. The only slight drawback of this change is that it slightly complicates adding new pipeline stages, seeing as one has to explicitly update the control flow of the pipeline. This is a minor drawback, seeing as this is also how things work in the software world, and is easily addressed by helper methods. Likewise, this change also removes the (old) `pipeline.stage` operations, which mainly were introduced to facilitate lowering. This is no longer needed with the block-based pipeline, seeing as stage in- and outputs are clearly denoted by block inputs and `pipeline.stage` operations.
7b88525
to
d61fc0a
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.
I haven't reviewed the implementation but I think the representational change makes sense based on the previous discusssions.
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 didn't make it to the stuff under 'lib', but had some high-level comments which I wanted you to see. May not get to the rest until tomorrow.
// Returns the last stage in the pipeline. | ||
Block* getLastStage(); | ||
|
||
// Adds a new stage to this pipeline. It is the users responsibility to |
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.
Where? "Adds" -> "Append" would be clearer if I assume correctly.
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'd actually say 'add' is the correct term here; append would imply that this stage logically comes after all other stages, which is not true. Ordering/placement of the stage within the pipeline is up to the user.
%1 = comb.add %a0, %a1 : i32 | ||
pipeline.stage ^bb1 regs (%1, %a0, %go) pass () enable %go |
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.
Is it critical to specify the next block? Can't we just use the lexical order? Would there ever be a use for anything but? I think not requiring that could be dangerous in that one could assume it (and use getStage(i)
instead of getOrderedStage(i)
) and be correct 99.9% of the time but that behavior is not guaranteed.
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.
Personally i'd prefer to use blocks with explicit terminator destinations, i.e. stages are a linked list. It makes it easier/safer to insert new stages into a pipeline, querying successor stages (Block::getSinglePredecessor, Block::getSuccessor,...
) and I personally think lexical ordering really only is a benefit for human readability (which is not the goal of the IR). Stringing together stages with explicit next-stage destinations will be correct 100% of the time.
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.
Yeah, but it's just so unintuitive to have block 3 be the 5th pipeline stage. I'm fine with the successor blocks being explicit as long as the verifier checks that it's explicitly pointing to the next block. Again, I'm very concerned about quiet/sleeper bugs like the one I discuss above. I'm also concerned about the readability of the asm output.
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.
stages are a linked list.
You realize that lists of blocks within a region are implemented as a literal linked list, yes? I know that's not your point, but I'm just sayin'...
querying successor stages (
Block::getSinglePredecessor, Block::getSuccessor,...
)
I'm not entirely certain how those are implemented, but I would assume through some sort of OpInterface (BranchOpInterface
?) on the terminators which we should absolutely implement. I suspect there are some potentially useful data flow analysis upstream which use it.
Plus, there's always Block::getNextNode()
and Block::getPrevNode()
.
Stringing together stages with explicit next-stage destinations will be correct 100% of the time.
Define "correct"... It's correct assuming the user strings it together correctly. If they do the intuitive thing and just add a block to the end, it won't be "correct". By the same token, using the numerical order is always "correct" assuming the user inserts the new block at the proper place. If they don't, it's easy to discover the mistake even after lowering to HW... In the case where a user just adds a block but doesn't modify the successors properly, I'd assume it just disappears when lowering to HW.
}]; | ||
|
||
let arguments = (ins Variadic<AnyType>:$registers, Variadic<AnyType>:$passthroughs, I1:$enable); | ||
let successors = (successor AnySuccessor:$nextStage); |
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.
See my comment in the rationale.
"bool", | ||
"isLatencyInsensitive", (ins), | ||
/*methodBody=*/"", | ||
/*defaultImplementation=*/[{ |
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 know this is probably just the previous logic factored out, so this may not be relevant to this PR; but, what if some of the inputs/outputs are LI. In other words, what if it's mixed LI non-LI? Would it make sense to return false for both this and isLatencySensitive
below?
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.
That should in my mind be an illegal case - it's XOR for now. In general, all of this LI-interfaced stuff is extremely blurry to me and will have to be revised once we need it.
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.
It's blurry for me as well. We might consider ripping it out. Add it back later on with dc.value
interfaces.
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'd be in favor of that as well - for a follow-up PR, though!
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 didn't make it to the stuff under 'lib', but had some high-level comments which I wanted you to see. May not get to the rest until tomorrow.
%1 = comb.add %a0, %a1 : i32 | ||
pipeline.stage ^bb1 regs (%1, %a0, %go) pass () enable %go |
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.
stages are a linked list.
You realize that lists of blocks within a region are implemented as a literal linked list, yes? I know that's not your point, but I'm just sayin'...
querying successor stages (
Block::getSinglePredecessor, Block::getSuccessor,...
)
I'm not entirely certain how those are implemented, but I would assume through some sort of OpInterface (BranchOpInterface
?) on the terminators which we should absolutely implement. I suspect there are some potentially useful data flow analysis upstream which use it.
Plus, there's always Block::getNextNode()
and Block::getPrevNode()
.
Stringing together stages with explicit next-stage destinations will be correct 100% of the time.
Define "correct"... It's correct assuming the user strings it together correctly. If they do the intuitive thing and just add a block to the end, it won't be "correct". By the same token, using the numerical order is always "correct" assuming the user inserts the new block at the proper place. If they don't, it's easy to discover the mistake even after lowering to HW... In the case where a user just adds a block but doesn't modify the successors properly, I'd assume it just disappears when lowering to HW.
This comes "for free" when blocks are used as the Fair, correctness might not be the proper term. Regardless, I don't think lexical ordering is a compelling alternative when the current implementation has a lot more in common with CFGs, and thus may share analysis, traversal, ... |
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 only skimmed the PipelineToHW.cpp changes.
I still disagreed with the block/stage ordering issue, but for the purposes of making forward progress I'll let it go.
This commit refactors the pipeline dialect to be block-based. This brings a major representational change in the form of:
pipeline.stagesep
operations to separate stages. Instead, pipeline stages are defined by blocks.pipeline.stage
operations.Block
s) have no arguments. In the materialized form, stages have arguments.pipeline.stage
operations which infers whether to register a value or pass it directly (i.e. a wire) to the next stage.pipeline.unscheduled
: Unscheduled pipeline, essentially just a container of operations.pipeline.scheduled
: Scheduled pipeline, containing pipeline stages.The motivation for this change is to improve the hierarchy of the IR, instead of relying on lexical ordering. This change also allows for more natural traversal of stages (
Block
s), as well as dataflow analysis of the pipeline, which now is analogous to control flow analysis. The only slight drawback of this change is that it slightly complicates adding new pipeline stages, seeing as one has to explicitly update the control flow of the pipeline. This is a minor drawback, seeing as this is also how things work in the software world, and is easily addressed by helper methods.Likewise, this change also removes the (old)
pipeline.stage
operations, which mainly were introduced to facilitate lowering. This is no longer needed with the block-based pipeline, seeing as stage in- and outputs are clearly denoted by block inputs andpipeline.stage
operations.Old representation:
New representation