Implement combinational modular memory #78

jardhu · 2019-06-24T21:54:25Z

This PR creates a modular implementation of a combinational memory that should function exactly the same as the DualPortedMemory. It will interface with the pipeline in the exact same manner as the non-combinational memory, and I included a tester for the memory which utilizes a CombinMemoryTestHarness that is structurally the same as the NonCombinMemoryTestHarness.

I also moved all memory component files into their own directory and renamed modules from the "asynchronous"/"synchronous" naming scheme to "NonCombin"/"Combin" respectively.

There is one issue with the implementation though, and that is that moving the masking logic for writing data into the data memory port introduces a combinational loop. I'm trying to find an elegant way around this that avoids including data manipulation code in the backing memory.

One last thing I want to do is to change the MemoryUnitTest into a testing suite that sets up a CombinMemoryUnitTest and NonCombinMemoryUnitTest (and any other future memory tests), and runs them all. This is so we can test all memory components in one single request, or run a CombinMemoryUnitTest or NonCombinMemoryUnitTest individually if needed. This might be out of scope, but it should be a small change to the code.

nganjehloo · 2019-06-25T03:14:11Z

For the combinational loop issue, I think having so many nest bundles for IO is causing this issue. I don't think there is a real loop based off the code I've read. (although i've been known to be wrong 99% of the time :P) You will have to split the signals in a more explicit manner so that they can be differentiated from each other more easily. I think the layers of bundles used to build a Request bundle is the cause of the issue. Also try to be explicit with what is an input and what is an output in your chisel. There is a way to force ignore the loop warning, but lets not do that just incase.

jardhu · 2019-06-30T05:07:14Z

I was able to get the combinational loop out, it was a bit more complicated than splitting the signals. Whenever we go into a when block with io.dmem.request.valid we can't use any of the request's other signals or else we end up introducing a combinational loop. So I basically pulled out the code that wires the memory out of the when loop. This fix relies on the fact that when the pipeline isn't supplying a request the wires in the memory/port buses are all set to DontCare, so the memory conveniently doesn't do anything when no memory operation is being done, but as I see it, this code would be fragile if we were to send unintended signals.

Ideally I would like for the memory to be enabled whenever io.dmem.request.valid is true just to make sure that the backing memory won't do anything when there is no valid request, but I'm unable to find some kind of enable feature in the Mem module. I think maybe I can force the memory's signals to be DontCare if io.dmem.response.valid is low.

powerjg

This is looking great!

One thing that we need to do is write some documentation. This interface is getting complicated (which seems to be required, though). We need some pictures and a clear description of what are the interfaces and how students (and others) should interact with this code.

Things we need to document

What is a port? It's a "smart" component that connects the pipeline to the memory.
- What "smarts" are in the port?
- Why different kinds of ports?
- What are the interfaces?
What are the different kinds of memories?
- When should you use each one
- How to configure the system for each one
What are the stable interfaces?
How can this be extended (e.g., what to modify to make a cache?)

Note: this documentation can be in another patch :).

powerjg · 2019-07-05T15:41:30Z

src/main/scala/components/memory/base-memory-components.scala

+/**
+  * Base class for all modular backing memories. Simply declares the IO and the memory file.
+  */
+class BaseDualPortedMemory(size: Int, memfile: String) extends Module {


Are these supposed to be abstract? I don't know exactly how to do this in scala, but maybe we should make it more explicit that these shouldn't be instantiated.

I suppose so, but I observed that the BaseBranchPredictor module isn't abstract as well so I figured that maybe there was some intention for this. If this shouldn't be the case then I'll correct both the base memory modules and the base branch predictor accordingly.

Right... Let's do this in a separate patch. I'd like to see this merged sooner rather than later.

For the branch predictor... I was on a deadline and couldn't figure out how to make the class abstract, IIRC. ;)

powerjg · 2019-07-05T16:10:15Z

src/main/scala/components/memory/memory-port-bus-io.scala

+class Request extends Bundle {
+  val address      = UInt(32.W)
+  val writedata    = UInt(32.W)
+  val operation    = UInt(1.W)


I think this needs to be two bits.

Is there a way to type it as a MemoryOperation since you have the enum? There should be a way to infer the width (in case we add another operation type).

powerjg · 2019-07-05T16:12:19Z

src/main/scala/components/memory/memory-combin-ports.scala

+
+  // When the memory is outputting a valid instruction
+  io.good := io.response.valid
+  when (io.response.valid) {


This when is probably not necessary. The pipeline should ignore the io.instruction if !io.good

powerjg · 2019-07-05T16:22:10Z

src/main/scala/components/memory/memory-combin-ports.scala

+
+        val data = Wire (UInt (32.W))
+        // Mask the portion of the existing data so it can be or'd with the writedata
+        when (io.maskmode === 0.U) {


I assume the plan is to split this logic out into a function in the BaseDMemPort in another patch since the code is replicated in NonCombinMemPort.

powerjg · 2019-07-05T16:22:28Z

src/main/scala/components/memory/memory-combin-ports.scala

+    io.request.valid := true.B
+
+    when (io.memwrite) {
+      // We issue a ReadWrite to the backing memory.


This all makes sense to me.

powerjg · 2019-07-05T16:25:14Z

src/main/scala/components/memory/base-memory-components.scala

+  * Base class for all modular backing memories. Simply declares the IO and the memory file.
+  */
+class BaseDualPortedMemory(size: Int, memfile: String) extends Module {
+  def wireMemPipe(portio: MemPortBusIO): Unit = {


This function is only used in the DualPortedCombinMemory, right? The NonCombin version has a different version (with a different signature) of this function. I think it would be better to move this into DualPortedCombinMemory

powerjg · 2019-07-05T16:34:12Z

src/main/scala/components/memory/memory.scala

+    // Check that address is pointing to a valid location in memory
+    assert (request.address < size.U)
+  } .otherwise {
+    memAddress := DontCare


Hmm... This is strange. I would have expected this to also introduce a combinational loop (or we still have the possibility that we're writing bogus data to memory).

Note: I tried just moving the "read path" and "write path" above into the when (io.dmem.request.valid) statement and it "just worked" for me. I don't see a combinational loop.

powerjg

I believe this is now good to go. There's a bit that needs to be cleaned up, but we can do that as the pipeline is updated.

Jared Barocsi added 5 commits June 20, 2019 08:46

Split operations into new file and add ReadWrite operation

c0d2dfa

Split AsyncMemIO into its own file and rename to MemPortBusIO

85c0616

Push initial impl of combinational memory

a9b7486

Push combinational memory ports

b9a661d

Implement combinational memory unit tests

14ffee5

Jared Barocsi added 3 commits June 29, 2019 14:57

Fix DualPortedCombinMemory combinational loop with ugly hack

a958073

Make combinational data memory more robust

4a9c083

Correct memory port outputting high for good when writing

ad92a3b

Implement memory module getters for the CPU config

fd52d6f

powerjg reviewed Jul 5, 2019

View reviewed changes

Jared Barocsi added 4 commits July 9, 2019 14:14

Initial attempt to move CPU models to modular memory

1128aa4

Commit minor code review changes

ffd0c97

Convert MemoryOperation into a strongly typed ChiselEnum

4d0edaa

Revert top file back to old memory for future patch

6c25396

powerjg mentioned this pull request Jul 9, 2019

Add cache #28

Open

9 tasks

powerjg approved these changes Jul 10, 2019

View reviewed changes

jardhu merged commit 9e9404a into jlpteaching:master Jul 10, 2019

jardhu deleted the combinational-memory branch July 10, 2019 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement combinational modular memory #78

Implement combinational modular memory #78

jardhu commented Jun 24, 2019

nganjehloo commented Jun 25, 2019 •

edited

Loading

jardhu commented Jun 30, 2019 •

edited

Loading

powerjg left a comment

powerjg Jul 5, 2019

jardhu Jul 9, 2019

powerjg Jul 9, 2019

powerjg Jul 5, 2019

powerjg Jul 5, 2019

powerjg Jul 5, 2019

powerjg Jul 5, 2019

powerjg Jul 5, 2019

powerjg Jul 5, 2019

powerjg left a comment

Implement combinational modular memory #78

Implement combinational modular memory #78

Conversation

jardhu commented Jun 24, 2019

nganjehloo commented Jun 25, 2019 • edited Loading

jardhu commented Jun 30, 2019 • edited Loading

powerjg left a comment

Choose a reason for hiding this comment

Things we need to document

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

powerjg left a comment

Choose a reason for hiding this comment

nganjehloo commented Jun 25, 2019 •

edited

Loading

jardhu commented Jun 30, 2019 •

edited

Loading