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.

Sign up for GitHub

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

Jump to bottom

New mem ctrlr classes #19

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

New mem ctrlr classes #19

wants to merge 15 commits into from

Conversation

mbabaie
Copy link
Collaborator

@mbabaie mbabaie commented Apr 21, 2022

No description provided.

mbabaie added 5 commits April 14, 2022 10:22
@mbabaie
mem: building anew branch on top of develop
dcb49ef 
Change-Id: If6deb4c472381f605b6c5a2db0925e5aaf2ae7d6
@mbabaie
mem: first version of 1st class (either DRAM or NVM)
c3f9c3e 
Change-Id: I8e3b76214d1fb1d85770466e644f59de26f9c386
@mbabaie
mem: first class implementation done
019663e 
Change-Id: I734e1087cc109f8efe00f4a28656e2de35f6f8cb
@mbabaie
mem: first class implementation done
3801551 
Change-Id: Ic9998ef8c14d804b9ef970b2a53fbd75140e79d2
@mbabaie
mem: first version of second class (2 interfaces)
a01a020 
Change-Id: I999a7f15b795a97da247f14b5069cec5143c4945
@mbabaie mbabaie force-pushed the new_memCtrlr_classes branch from 5a004d4 to a01a020 Compare April 21, 2022 17:15
mbabaie added 3 commits April 21, 2022 17:22
@mbabaie
mem: splited mem ctrls and mem intrfcs in separate files
4e307ba 
Change-Id: I18ef79b93730e752c86dc5e92519f4d20814815f
@mbabaie
mem: applied almost all of the points discussed for base ctrlr
b2698df 
Change-Id: I859876a70e0bf9568ca661c1312d188c05a0d307
@mbabaie
mem: more updates on the structures and names and reusing
00b1934 
Change-Id: Ia4931be8b8de3e6406fd8e72d7639c4950386858
powerjg
powerjg reviewed Apr 26, 2022
View reviewed changes
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.

A couple of small comments. For others, I'll need to download and run some diffs... let me know when you're ready for another careful review :)

src/mem/MemCtrl.py Show resolved Hide resolved
src/mem/SimpleMemCtrl.py Outdated Show resolved Hide resolved
mbabaie and others added 7 commits April 26, 2022 13:59
@mbabaie
mem: cleaned the commented lines
9ccd369 
Change-Id: Ib4ae7112c7a22c9c18ad4ec65105c246575f7895
@mbabaie
mem: cleaned the commented lines
80e18e9 
Change-Id: Ie8acf90f91b3a4fedfcc6080918bbecf1c2bdab7
@mbabaie
mem: lates version
7ee12bb 
Change-Id: I027af55c0e74e62562d290b8e2f97ae8feb21196
@mbabaie
mem: temporarily added trafGen script
8f02f6b 
Change-Id: I75716ee3fbbefdc6a1152fad92851de42c4aa1e9
@mbabaie
Delete traffGen.py
1bffb46
@mbabaie
mem: latest version with primary testing: identical results
3259752 
Change-Id: Idc741c3867b4ab8152e4544864767eed8f72da1d
@mbabaie
mem: latest version with primary testing: identical results
5750029 
Change-Id: I412a3f6431272a7927fc934314d435590a58adbc
powerjg
powerjg requested changes May 4, 2022
View reviewed changes
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.

Lots of comments, though most are small :). I would suggest holding off working on this until after I have a look at Ayaz's changes, too. I'd like to see how they fit in.

src/mem/SConscript Show resolved Hide resolved
src/mem/MemCtrl.py Show resolved Hide resolved
src/mem/MemCtrl.py
# effects of a memory controller, interfacing with media specific
# interfaces
class MemCtrl(QoSMemCtrl):
class MemCtrl(SimpleMemCtrl):
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 comment which says it has two interfaces and a bit more detail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in #23

src/mem/mem_interface.hh
@@ -180,13 +177,17 @@ class MemInterface : public AbstractMemory
const uint32_t readBufferSize;
const uint32_t writeBufferSize;

uint32_t numWritesQueued;

bool isDramIntr;
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I explained this in simple_mem_ctrl.cc line 253.

src/mem/nvm_interface.hh Show resolved Hide resolved
src/mem/mem_ctrl.cc
@@ -823,24 +385,23 @@ MemCtrl::doBurstAccess(MemPacket* mem_pkt)
// When was command issued?
Tick cmd_at;

std::vector<MemPacketQueue>& queue = selQueue(mem_pkt->isRead());
Copy link
Member

Choose a reason for hiding this comment

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

Unneeded change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apparently it's fixed already (Maybe by Ayaz, while he was merging his code with mine)

src/mem/mem_ctrl.cc
dram->bytesPerBurst() :
nvm->bytesPerBurst()) );
dram->bytesPerBurst() :
nvm->bytesPerBurst()));
Copy link
Member

Choose a reason for hiding this comment

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

Whitespace changes. Please revert.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apparently it's fixed already (Maybe by Ayaz, while he was merging his code with mine)

src/mem/mem_ctrl.cc
@@ -1099,12 +657,13 @@ MemCtrl::processNextReqEvent()

// sanity check
assert(mem_pkt->size <= (mem_pkt->isDram() ?
dram->bytesPerBurst() :
nvm->bytesPerBurst()) );
dram->bytesPerBurst() :
Copy link
Member

Choose a reason for hiding this comment

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

Whitespace changes. Please revert.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apparently it's fixed already (Maybe by Ayaz, while he was merging his code with mine)

src/mem/mem_ctrl.cc
// All other queues verified as needed with calling logic
bool nvm_drained = !nvm || nvm->allRanksDrained();
return (dram_drained && nvm_drained);
// ensure dram is in power down and refresh IDLE states
Copy link
Member

Choose a reason for hiding this comment

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

Whitespace changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed already.

src/mem/mem_ctrl.cc

bool
MemCtrl::MemoryPort::recvTimingReq(PacketPtr pkt)
std::vector<MemInterface*>
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure this can be removed and replaced with an implementation of getAddrRanges

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aakahlow aakahlow aakahlow left review comments

@powerjg powerjg powerjg requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mbabaie @powerjg @aakahlow