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

Dram Cache Controller (Hit (rd/wr)+ Miss (rd)) #4

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from
Open

Conversation

mbabaie
Copy link
Collaborator

@mbabaie mbabaie commented Oct 4, 2021

No description provided.

aakahlow and others added 21 commits February 23, 2021 23:03
Change-Id: Iacad811fb48cf65be8cfadab7fdb421ad0c98a6b
Change-Id: I3b782f6d72ef64a96d7a2d3133b745dc99d7a025
Change-Id: If38a31c4ba0af68d7b90a8cb2ee99a364b66d561
Change-Id: Ifbfbf4aff79cbab758a35ec2ba07c19fb0b99d43
Change-Id: I3d335f8c64a605c24c0136c06087e41779e5ea30
Change-Id: Ic50fab42b25fcd98d32fab5d849e896882f82d02
Change-Id: If79e442f3c562ab0455f1c35146a032524d4ec9b
Change-Id: Ie7f78f53c5a20b42ff73c1459aaff551a9de1705
Change-Id: Iea35abd1d85d22731b6859940517642420411452
Change-Id: I724a84be596e84327488f485ded727f69e60ced2
Change-Id: I7812f21e19552aab60e003d57ae1b76fc1bf6769
Change-Id: I56d5178272a10f1d811de721c4c19962f4fd2abb
Change-Id: I4bea6ea7b61ad7ca404016e6f584c7a6be61b1f0
Change-Id: I888dcda7b7dbbaee6e39dce66245a2913b48817d
Change-Id: Ib1cc88d8daab2a896b848d41ff622a0f4ea99d1f
Change-Id: Id7a9742a140d9bd8de7f01326ce0181129d902de
Change-Id: I25f6dcc7bbf5b3057709633c4a8899aa518dbaf9
@mbabaie mbabaie changed the title Hit Only Dram Cache Controller Dram Cache Controller (Hit (rd/wr)+ Miss (rd)) Oct 5, 2021
Change-Id: I611cf06579ecb47e239d3b43163764f0e3a4abeb
Copy link
Member

@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 don't think it's feasible for me to review the dcache_ctrl and dcmem_interface as there's no way for me to tell what's new vs. what came from the original file. We need to do something to reduce the code duplication between the DRAM models and the DRAM cache models before a review is feasible.

Below are a few comments. Make sure you put copyrights in all files that you create/modify.

@@ -0,0 +1,75 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

This file shouldn't be committed.

I strongly suggest creating a PR like this, then looking at it yourself and doing a "self review" to fix some of these low hanging fruits

simple.py Outdated
system.generator.port = system.mem_ctrl.port

def createRandomTraffic(tgen):
yield tgen.createRandom(100000000000, # duration
Copy link
Member

Choose a reason for hiding this comment

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

I know you're changing this a lot. I would suggest making this an argument to the script with optparse

m5.instantiate()
system.generator.start(createRandomTraffic(system.generator))
#system.generator.start(createLinearTraffic(system.generator))
exit_event = m5.simulate()
Copy link
Member

Choose a reason for hiding this comment

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

This needs a newline at the end of the file. You should configure your editor to automatically add newlines at the end of the file and remove all trailing whitespace.

Comment on lines +51 to +52
system.generator.start(createRandomTraffic(system.generator))
#system.generator.start(createLinearTraffic(system.generator))
Copy link
Member

Choose a reason for hiding this comment

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

This should be an option, too.

Whatever things in this script that you want to change frequently should be command line options. That's the purpose of command line options :)

@@ -0,0 +1,66 @@
from m5.params import *
Copy link
Member

Choose a reason for hiding this comment

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

Needs a copyright.

Comment on lines +27 to +28
# JASON: let's get this size from the interface
dram_cache_size = Param.MemorySize('1024MiB', "DRAM cache size")
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget this

Comment on lines +22 to +25
dram = Param.DRAMDCInterface(NULL, "DRAM interface")

# Interface to non-volatile media
nvm = Param.NVMDCInterface(NULL, "NVM interface")
Copy link
Member

Choose a reason for hiding this comment

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

I believe both of these are required. They shouldn't default to NULL

@@ -0,0 +1,59 @@
#Copyright (c) 2021 The Regents of the University of California.
Copy link
Member

Choose a reason for hiding this comment

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

What is this file? What is DcacheCtrl? Which one are you using?

@@ -0,0 +1,69 @@

Copy link
Member

Choose a reason for hiding this comment

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

Must have copyright

@@ -350,6 +350,8 @@ class Packet : public Printable
*/
PacketDataPtr data;

PacketDataPtr cacheLineRead;
Copy link
Member

Choose a reason for hiding this comment

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

Why are these changes needed? It would be best if packet doesn't need to be changed.

Change-Id: I335c790c560a023353981f19f119b6adcb2b18ef
Change-Id: I331ee5f0cffe109f2a766a7f0052dd9614bd1c01
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