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

Lab3 #12

Closed
wants to merge 11 commits into from
Closed

Lab3 #12

wants to merge 11 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 30, 2019

No description provided.

@ghost ghost requested a review from powerjg January 30, 2019 23:14
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.

I have quite a few comments on style. It might seem silly, but we need to lead by example and show the students how to write high quality code. We must follow style guidelines.

Otherwise, most of the changes seem fine to me. If you can go through my comments and either make the changes or respond the the comment by Friday evening, I'd appreciate it. I'll spend some time Saturday or Sunday working on writing up the rest of the lab.

val forwardA = Output(UInt(2.W))
val forwardB = Output(UInt(2.W))
})

//printf("rs1 = %d, rs2 =%d, exmemrd = %d, exmemrw = %d,memwbrd = %d, memwbrw = %x,forwardA = %d, forwardB =%d ",io.rs1,io.rs2,io.exmemrd,io.exmemrw,io.memwbrd,io.memwbrw,io.forwardA,io.forwardB)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't leave things commented out. Either delete the line, uncomment the line, or add a flag to enable it at runtime.

when (io.rs1 === io.exmemrd && io.exmemrd =/= 0.U && io.exmemrw) {
io.forwardA := 1.U
} .elsewhen (io.rs1 === io.memwbrd && io.memwbrd =/= 0.U && io.memwbrw) {
} .elsewhen (io.rs1 === io.memwbrd && io.memwbrd =/= 0.U && io.memwbrw && ~(io.exmemrw && io.exmemrd =/=0.U && io.exmemrd ===io.rs1)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't make sense to me. Isn't it the same as in the when clause?

io.forwardA := 2.U
} .otherwise {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this compile? Isn't this an extra }

io.forwardA := 0.U
}

when (io.rs2 === io.exmemrd && io.exmemrd =/= 0.U && io.exmemrw) {
io.forwardB := 1.U
} .elsewhen (io.rs2 === io.memwbrd && io.memwbrd =/= 0.U && io.memwbrw) {
} .elsewhen (io.rs2 === io.memwbrd && io.memwbrd =/= 0.U && io.memwbrw&& ~(io.exmemrw && io.exmemrd =/=0.U && io.exmemrd ===io.rs2)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto above. This shouldn't be needed, unless I'm missing something.

io.forwardB := 2.U
} .otherwise {
}.otherwise {
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this line. Why did you get rid of this space?

@@ -39,6 +40,7 @@ addi a5,a5,1
sw a5,0x500(zero)

label2:
nop
Copy link
Contributor

Choose a reason for hiding this comment

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

Needed???




val forward_inputx = Wire(UInt(32.W))
Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace?

ex_mem.mcontrol.jump := id_ex.mcontrol.jump

printf("branch control taken = %d\n", branchCtrl.io.taken)
printf("jump = %d\n", id_ex.mcontrol.jump)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these prints necessary?

@@ -253,8 +339,9 @@ class PipelinedCPU(implicit val conf: CPUConfig) extends Module {
(mem_wb.wbcontrol.toreg === 0.U) -> mem_wb.aluresult,
(mem_wb.wbcontrol.toreg === 1.U) -> mem_wb.readdata,
(mem_wb.wbcontrol.toreg === 2.U) -> mem_wb.pcplusfour))

printf("write_data = %d\n", write_data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Needed?

_last:


.data
.byte 0xFF,0xFF,0xFF,0xFF
.word 4294967295
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change?

powerjg and others added 2 commits January 31, 2019 14:48
@powerjg
Copy link
Contributor

powerjg commented Feb 1, 2019

@FilipeEduardo, I just tried to build/run the tests, but I'm receiving a ton of errors. It looks like there's a cycle in the combinational logic.

Thanks!
Jason

@ghost
Copy link
Author

ghost commented Feb 1, 2019

@FilipeEduardo, I just tried to build/run the tests, but I'm receiving a ton of errors. It looks like there's a cycle in the combinational logic.

Thanks!
Jason

I ran all the tests again to see which ones fail. The only tests that fail are the multicycle instructions for the five-cycle CPU. All tests pass for the Pipelined CPU.

@powerjg
Copy link
Contributor

powerjg commented Feb 3, 2019

I think you forgot to commit something. I'm seeing "Tests: succeeded 3, failed 68, canceled 0, ignored 0, pending 0"

@powerjg
Copy link
Contributor

powerjg commented Feb 3, 2019

Nevermind. when I run "Lab3 / test" it works, but when I run "test" it doesn't. What tests broke?

Signed-off-by: Jason Lowe-Power <[email protected]>
@powerjg
Copy link
Contributor

powerjg commented Feb 4, 2019

I fixed the register file, btw. I used a scala if to check if we were running the single cycle or the pipelined/five-cycle versions.

I'm going to close this PR and delete this branch so I can use the branch name for something else.

Thanks!

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.

1 participant