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

[Pipeline] Add optional stall signal to pipelines #5278

Merged
merged 2 commits into from
Jun 13, 2023

Conversation

mortbopet
Copy link
Contributor

This signal is intended to connect to all stages within the pipeline, and is used to stall the entirety of the pipeline. It is lowering defined how stages choose to use this signal, although in the common case, a stall signal would typically connect to the clock-enable input of the stage-separating registers.

Future PRs will implement hardware lowerings - blocked by #5234.

@mortbopet
Copy link
Contributor Author

CC @blakep-msft.

@teqdruid
Copy link
Contributor

teqdruid commented Jun 7, 2023

This is one style of stalling. It's my understanding that what we actually want is to stall only the stages which cannot proceed without clobbering the next stage. Meaning if the next stage doesn't have valid data in it, the current stage doesn't need to stall. @blakep-msft, is my understanding correct? If so, does this support that style?

@blakep-msft
Copy link
Contributor

This is one style of stalling. It's my understanding that what we actually want is to stall only the stages which cannot proceed without clobbering the next stage. Meaning if the next stage doesn't have valid data in it, the current stage doesn't need to stall. @blakep-msft, is my understanding correct? If so, does this support that style?

I believe this PR just adds the stall input signal, which should be fine. The interesting cases will come up with the lowerings. If a pipeline has some stages which are stallable, and others which are not then you can have clobbering problems. One solution is to have stallable stages advance in spite of a high stall signal, in the case where the nearest upstream non-stallable stage has valid data in it. Representing this as a chain of pipelines (each pipeline in the chain can be stallable or not) may be a workable approach.

Copy link
Contributor

@teqdruid teqdruid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blake makes a good point -- it's all in the lowering

@mortbopet
Copy link
Contributor Author

That was also my view of the problem - a "stall this pipeline" signal seems generic enough to apply to all cases. Now, how that stall is actually implemented in hardware is a separate issue.

This signal is intended to connect to all stages within the pipeline, and is used to stall the entirety of the pipeline. It is lowering defined how stages choose to use this signal, although in the common case, a `stall` signal would typically connect to the clock-enable input of the stage-separating registers.

Future PRs will implement hardware lowerings - blocked by #5234.
@mortbopet mortbopet force-pushed the dev/mpetersen/pipeline_stall branch from ebf8df5 to ae8a641 Compare June 12, 2023 09:41
@mortbopet mortbopet merged commit 4271177 into main Jun 13, 2023
@mortbopet mortbopet deleted the dev/mpetersen/pipeline_stall branch June 23, 2023 08:16
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