ext: Update DRAMPower

Information
Submitter:	Matthias Jung
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	andysan

Description

Sync DRAMPower to external tool

This patch syncs the DRAMPower library of gem5 to the external
one on github (https://github.com/ravenrd/DRAMPower) of which
I am a maintainer.

The version used is the commit:
902a00a1797c48a9df97ec88868f20e847680ae6
from 07. May. 2016.

Testing Done

Everything compiles.

Issue Summary

Description	From	Last Updated	Status
this is rather unconventional max<int64_t>(0, ...) the construct appears in a few places	Andreas Hansson	July 1, 2016, 10:09 a.m.	Dropped
really no point making the int parameter const	Andreas Hansson	July 1, 2016, 10:09 a.m.	Dropped
cstdint rather?	Andreas Hansson	July 1, 2016, 10:09 a.m.	Dropped
why are these signed?	Andreas Hansson	July 1, 2016, 10:09 a.m.	Dropped
max<int64_t>(...	Andreas Hansson	July 1, 2016, 10:09 a.m.	Dropped

Change Summary:

Summary Changed

Summary:

-	mem: Update DRAMPower
+	ext: Update DRAMPower

Change Summary:

Description Changed

Description:

~		This patch syncs the DRAMPower library of gem5 to the external one on github (https://github.com/ravenrd/DRAMPower) of which I am a maintainer.
~		The version used is commit 902a00a1797c48a9df97ec88868f20e847680ae6 from 07. May. 2016.
	~	Sync DRAMPower to external tool
	~
	+	This patch syncs the DRAMPower library of gem5 to the external
	+	one on github (https://github.com/ravenrd/DRAMPower) of which
	+	I am a maintainer.
	+
	+	The version used is the commit:
	+	902a00a1797c48a9df97ec88868f20e847680ae6
	+	from 07. May. 2016.

Ship It!

I realise I am commenting on DRAMPower changes here, so feel free to ignore.

There seems to be an awful lot of changes from unsigned to signed types, which feels rather unintuitive. Why not uint64_t if the value is truly unsigned? It seems odd to remove that clear communication of intent.

There also seems to be quite a few static_cast<size_t> cause by the above change, which clutters the code. If the bank, for example, is unsigned this should not be needed.

Matthias Jung June 26, 2016, 10:15 p.m. (June 26, 2016, 10:15 p.m.)

there where some discussions about that issue in DRAMPowers git repro:

https://github.com/ravenrd/DRAMPower/issues/10#issuecomment-207067743

Andreas Hansson June 30, 2016, 12:20 a.m. (June 30, 2016, 12:20 a.m.)

I am not against updating, but some of these style issues would be good to get resolved as even clang and gcc complain.

Matthias Jung June 30, 2016, 2:10 a.m. (June 30, 2016, 2:10 a.m.)

I discussed that internally with Sven, Benny and Éder that we will include these suggestions in the release of DRAMPower v4.1.
Since we have other pending tasks for this release, it will take some more time, which would keep this patch ideling on the review board.
I would suggest that this patch is pushed to gem5 now in order to include the latest improved version of DRAMPower and we will come up with v4.1. as fast as possible.

Radhika Jagtap June 30, 2016, 6:05 a.m. (June 30, 2016, 6:05 a.m.)

Indeed, would be good to get the 4.0 upstreamed to gem5 because it adds the clearCounters method which is sort of a bug fix if you do intermediate stats dump (https://github.com/ravenrd/DRAMPower/issues/31)

ext/drampower/src/CmdScheduler.cc (Diff revision 1)

this is rather unconventional
max<int64_t>(0, ...)
the construct appears in a few places

Show all issues

ext/drampower/src/CommandAnalysis.h (Diff revision 1)

really no point making the int parameter const

Show all issues

ext/drampower/src/MemArchitectureSpec.h (Diff revision 1)

cstdint rather?

Show all issues

ext/drampower/src/MemArchitectureSpec.h (Diff revision 1)

why are these signed?

Show all issues

ext/drampower/src/MemCommand.cc (Diff revision 1)

max<int64_t>(...

Show all issues

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Change Summary:

Summary:

Change Summary:

Description:

Status: Closed (submitted)