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

Add non-combinational CPU model #87

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

jardhu
Copy link
Collaborator

@jardhu jardhu commented Sep 20, 2019

This pull request implements a modified CPU model that accounts for memory stalling. In particular a new PipelinedNonCombinCPU class is added containing the design of this CPU, as well as a new hazard detection unit with signals for stalling the pipeline when the memory is busy.

Likewise new functionality was implemented for the testing drivers to allow the tester driver to specify what type of memory and ports are required for the test. This is used primarily to perform the instruction tests and full application tests for the non combinational CPU.

All existing test cases should default to using the combinational memory and combinational ports and retain the previous testing behavior, at least from what I tested.

Jared Barocsi added 6 commits September 17, 2019 10:49
Update master with new memory changes
This signal serves as a way for the pipeline to know whether the backing memory
is ready for another request. This is required for the non-combinational memory
to stall when the memory is not ready to continue.

Previously, the 'good' signal served this purpose but is now used to write valid
memory data to each stage's register.
@jardhu
Copy link
Collaborator Author

jardhu commented Sep 20, 2019

This should cover all of the new changes I had in the previous pull request with a cleaner commit history and none of the previous polluting commits. So, right now I just need to change the hazard detection unit and CPU to use the good/ready signals properly, and hopefully it should work.

@powerjg
Copy link
Contributor

powerjg commented Sep 20, 2019 via email

@jardhu
Copy link
Collaborator Author

jardhu commented Sep 24, 2019

I'm having some trouble trying to get the non-combinational CPU to work with the signals; nothing I did before seems to work now. Could I perhaps talk with you about it on Wednesday?

@jardhu
Copy link
Collaborator Author

jardhu commented Oct 4, 2019

Issue with JAL:
The PC correctly jumps to the address specified by JAL, however the CPU does not properly "link" PC+4 to the registers. In the JAL tests, 8 is written instead of the correct value of 4.
Possible causes:
The CPU always starts with imem being busy, despite there being no incoming instruction or signal to do so.

Methodology to reproduce:

  1. Clone or pull the changes up to 2201ebf.
  2. Run testOnly dinocpu.PipelinedNonCombinCPUTester -- -z jal in sbt.
  3. Some debugging statements should appear. Most notably, at the beginning,
IMEM READY: 0 DMEM READY: 1
Cycle 1: PC=>4 (8)
IMEM READY: 0 DMEM READY: 1
Cycle 2: PC=>4 (8)
IMEM READY: 0 DMEM READY: 1
Cycle 3: PC=>4 (8)
IMEM READY: 0 DMEM READY: 1
Cycle 4: PC=>4 (8)
IMEM READY: 1 DMEM READY: 1
---------------------------------------------
Cycle 5: PC=>4 (12)
IMEM READY: 0 DMEM READY: 1
Cycle 6: PC=>8 (12)
IMEM READY: 0 DMEM READY: 1
Cycle 7: PC=>8 (12)
IMEM READY: 0 DMEM READY: 1
Cycle 8: PC=>8 (12)
...```
On every cycle PC (after the arrow) and PC+4 (in the parenthesis) are displayed, as well as the states of memory below it. Lines are outputted whenever the CPU is not stalling to indicate a new memory request being pushed through.

Jared Barocsi added 11 commits November 22, 2019 19:43
This signal serves as a way for the pipeline to know whether the backing memory
is ready for another request. This is required for the non-combinational memory
to stall when the memory is not ready to continue.

Previously, the 'good' signal served this purpose but is now used to write valid
memory data to each stage's register.
This will probably be rebased so that it's merged in the commit that actually
adds this class
@jardhu
Copy link
Collaborator Author

jardhu commented Nov 23, 2019

Nothing new here, I'm just updating my branch with all the changes to master :)

@jardhu jardhu changed the base branch from master to update-memportio November 23, 2019 04:00
@jardhu jardhu changed the base branch from update-memportio to master November 23, 2019 04:01
@jardhu
Copy link
Collaborator Author

jardhu commented Nov 23, 2019

One minor comment about the base branch changes: Apparently, it is by design that a feature branch with merged-in master commits shows these old commits in a pull request. Obviously we do not want these in the pull request's history, so a workaround to remove these is to change the base of the PR from master to a temporary branch and change it back.

Copy link
Contributor

@powerjg powerjg left a comment

Choose a reason for hiding this comment

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

Looking good! What's the current state? Does the new design pass the tests?

* Output: exmem_disable, if true, explicitly prevent the EX/MEM register from being written to
*
* For more information, see Section 4.7 and beginning of 4.8 of Patterson and Hennessy
* This follows the "Data hazards and stalls" section and the "Assume branch not taken" section
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this true or copied from somewhere else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new signals do not follow the pattern. Do you think I should move them beyond that note about the Patterson note and clarify that they are additionally added in?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say just drop that part of the comment if it doesn't apply anymore. It's fine to go beyond the textbook :)

@jardhu
Copy link
Collaborator Author

jardhu commented Jan 3, 2020

R-type and I-type tests work. Branching and jumps still don't work, and memory does not work. By extension the small applications tests also fails.

I am working on the memory tests right now.

@powerjg
Copy link
Contributor

powerjg commented Jan 3, 2020

BTW, Sorry if my recent commits cause any conflicts. However, if you rebase on master and update the file to work with the new single stepper printing system you should be able to debug more easily.

Copy link
Contributor

@powerjg powerjg left a comment

Choose a reason for hiding this comment

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

A couple of small FYI's :)

@@ -15,7 +15,10 @@ class CPUFlatSpec extends FlatSpec with Matchers
class CPUTesterDriver(cpuType: String,
branchPredictor: String,
binary: String,
extraName: String = "") {
extraName: String = "",
forceDebug: Boolean = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

force debug was removed in master, fyi.

val cpustr = if (branchPredictor != "") { cpuType+"-bp" } else { cpuType }
val driver = new CPUTesterDriver(cpustr, branchPredictor, testCase.binary, testCase.extraName)
val driver = new CPUTesterDriver(cpustr, branchPredictor, testCase.binary, testCase.extraName, false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need false here anymore.

@powerjg
Copy link
Contributor

powerjg commented Jan 11, 2020

Getting closer :)

Jared Barocsi and others added 6 commits January 14, 2020 08:08
- Add dump function to the single stepper
- Improve the instruction print function

Still need to make the pipeline registers print nicely

Closes jlpteaching#107
Closes jlpteaching#59

Signed-off-by: Jason Lowe-Power <jason@lowepower.com>
Closes jlpteaching#108

Signed-off-by: Jason Lowe-Power <jason@lowepower.com>
@powerjg
Copy link
Contributor

powerjg commented Jan 14, 2020

Looking good! It looks like it's pretty close!

@jardhu
Copy link
Collaborator Author

jardhu commented Jan 18, 2020

@powerjg All instruction unit tests pass, only the simple applications fail now. I won't be available for the next few days to look closer at what is happening, but could you perhaps give some insight onto specific architectural details each application is supposed to test? Or are they designed so that if every single unit test works, then the simple applications also should pass after?

@powerjg
Copy link
Contributor

powerjg commented Jan 18, 2020

Well, the idea was that if you pass the instruction tests the full applications would work. However, in practice, it seems that there are some cases the instruction tests don't test well. We might be able to go back to Piazza from wq '19 and see what those cases were and develop some better tests :).

IIRC, the cases that weren't covered had to do with JAL and JALR, but I could be mis-remembering.

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.

2 participants