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

honor the 'ready' signal from the GCD output #153

Conversation

wunderabt
Copy link
Contributor

An attempt to fix issue #134

"// test for backpressure\n",
"test(new DecoupledGcd(16)) { dut =>\n",
" dut.input.initSource().setSourceClock(dut.clock)\n",
" dut.output.initSink().setSinkClock(dut.clock)\n",
Copy link
Contributor Author

@wunderabt wunderabt May 17, 2021

Choose a reason for hiding this comment

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

What I want to do is to use slower clock for the sink so that there is greater chance that a fork + join approach runs into a backpressure scenario but I don't know how to write a ClockDivider here? @edwardcwang or @ducky64 do you maybe have some directions?

" dut.input.enqueueNow(inputVal)\n",
" dut.output.ready.poke(false.B)\n",
" for(_ <- 0 to 10) {\n",
" dut.clock.step(1)\n",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is my poor mans solution for the clock divider - set the ouput in a non-ready state and idle for come cycles.

@jackkoenig
Copy link
Collaborator

Looks like #154 did fix CI for PRs from forks so that's good.

@chick
Copy link
Contributor

chick commented Jun 28, 2021

@wunderabt I think the clock divider approach has a lot of issues that would be overkill to try and solve in this context. As a simple example, take a look at example of backpressure/input-stall randomly adding extra steps in the dequeue block creates backpressure, adding extra steps in the enqueue block stalls input.

@edwardcwang
Copy link
Collaborator

I agree; just having the dequeue block artificially take longer to compute would be easier.

@chick
Copy link
Contributor

chick commented Aug 18, 2021

@wunderabt I am closing this. The GCD example produces back-pressure automatically as it takes input dependent number of multiple cycles to compute a single GCD. If you run the base standard example you can see that it takes ~600 cycles to compute 100 GCD values. This only works if the decoupled helpers are handling the back-pressure properly.
Maybe the text could make that more explicit, please propose some different text if you have a suggestion

@chick chick closed this Aug 18, 2021
@wunderabt
Copy link
Contributor Author

wunderabt commented Aug 20, 2021

Hi @chick thanks for looking into this! I agree to closing this PullRequest as it wasn't a good solution to the issue. But I'm still in doubt whether #134 is adressed yet.

On the input side the back-pressure is handled correctly - as you say, the computation takes time and while it's computing no new inputs are accepted via input.ready := ! busy.
But what about output.ready? The output side may not be ready to accept a new result, isn't it? The output.ready signal of the decoupled output interface is ignored. The vital change of this Pull-Request was to connect it like so:
https://github.com/freechipsproject/chisel-bootcamp/pull/153/files#diff-d0cf6f075ca780e47d3802aa0752cdb5f172ad6783391a517515039b8124cbe4R442

For me the question remains how the dut.output.expectDequeueSeq(resultSeq) can block the ready signal in a controlled way so that backpressure can be tested on the output. And in this example it may have to withhold the output.ready for a while because the GCD computation takes many cycles.

@wunderabt
Copy link
Contributor Author

@wunderabt I think the clock divider approach has a lot of issues that would be overkill to try and solve in this context. As a simple example, take a look at example of backpressure/input-stall randomly adding extra steps in the dequeue block creates backpressure, adding extra steps in the enqueue block stalls input.

I've taken that example and used it in #161

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.

4 participants