Discussion:
[gem5-dev] Review Request: mem: Add a requestor ID to each request object.
(too old to reply)
Ali Saidi
2012-02-04 20:29:11 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1026/
-----------------------------------------------------------

Review request for Default.


Description
-------

mem: Add a requestor ID to each request object.

This change adds a requestor id to each request object which can be
used identify every device in the system that is capable of issuing a request.
This is part of the way to removing the numCpus+1 stats in the cache and
replacing them with the requestor ids. This is one of a series of changes
that make way for the stats output to be changed to python.


Diffs
-----

src/arch/arm/isa.cc 223945df5510
src/arch/arm/table_walker.hh 223945df5510
src/arch/arm/table_walker.cc 223945df5510
src/arch/x86/intmessage.hh 223945df5510
src/arch/x86/pagetable_walker.hh 223945df5510
src/arch/x86/pagetable_walker.cc 223945df5510
src/cpu/base.hh 223945df5510
src/cpu/base.cc 223945df5510
src/cpu/base_dyn_inst.hh 223945df5510
src/cpu/checker/cpu.hh 223945df5510
src/cpu/checker/cpu.cc 223945df5510
src/cpu/checker/cpu_impl.hh 223945df5510
src/cpu/inorder/resources/cache_unit.cc 223945df5510
src/cpu/inorder/resources/fetch_unit.cc 223945df5510
src/cpu/inorder/resources/tlb_unit.hh 223945df5510
src/cpu/o3/fetch_impl.hh 223945df5510
src/cpu/simple/atomic.cc 223945df5510
src/cpu/simple/base.cc 223945df5510
src/cpu/simple/timing.cc 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.hh 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.cc 223945df5510
src/cpu/testers/directedtest/InvalidateGenerator.cc 223945df5510
src/cpu/testers/directedtest/RubyDirectedTester.py 223945df5510
src/cpu/testers/directedtest/SeriesRequestGenerator.cc 223945df5510
src/cpu/testers/memtest/MemTest.py 223945df5510
src/cpu/testers/memtest/memtest.hh 223945df5510
src/cpu/testers/memtest/memtest.cc 223945df5510
src/cpu/testers/networktest/NetworkTest.py 223945df5510
src/cpu/testers/networktest/networktest.hh 223945df5510
src/cpu/testers/networktest/networktest.cc 223945df5510
src/cpu/testers/rubytest/Check.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.hh 223945df5510
src/cpu/testers/rubytest/RubyTester.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.py 223945df5510
src/dev/io_device.hh 223945df5510
src/dev/io_device.cc 223945df5510
src/mem/cache/cache_impl.hh 223945df5510
src/mem/cache/prefetch/Prefetcher.py PRE-CREATION
src/mem/cache/prefetch/base.hh 223945df5510
src/mem/cache/prefetch/base.cc 223945df5510
src/mem/cache/tags/iic.cc 223945df5510
src/mem/port.cc 223945df5510
src/mem/request.hh 223945df5510
src/mem/ruby/recorder/CacheRecorder.cc 223945df5510
src/sim/system.hh 223945df5510
src/sim/system.cc 223945df5510

Diff: http://reviews.gem5.org/r/1026/diff/diff


Testing
-------

no stats changes on regression tests


Thanks,

Ali Saidi
Nathan Binkert
2012-02-06 17:35:20 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1026/#review2083
-----------------------------------------------------------



src/arch/arm/table_walker.hh
<http://reviews.gem5.org/r/1026/#comment2567>

Seems like a typedef for RequestID might be in order. I doubt that it really needs to be a uint32_t. Also, might having a -1 value be useful?



src/arch/x86/pagetable_walker.hh
<http://reviews.gem5.org/r/1026/#comment2568>

shouldn't this be a static_cast? We should always know the type. (It could be a safe_cast if you like)



src/cpu/testers/rubytest/Check.cc
<http://reviews.gem5.org/r/1026/#comment2569>

What's this?



src/mem/request.hh
<http://reviews.gem5.org/r/1026/#comment2570>

Maybe a little comment on what each of these rids are for? writeback, functional, and ????



src/mem/request.hh
<http://reviews.gem5.org/r/1026/#comment2571>

These changes are somewhat dangerous because of type conversions. A simple wrapper class for the RequestID would be zero code overhead, yet it would prevent accidents by preventing implicit casts and strange bugs. Just a thought.


- Nathan Binkert
Post by Ali Saidi
-----------------------------------------------------------
http://reviews.gem5.org/r/1026/
-----------------------------------------------------------
(Updated Feb. 4, 2012, 12:29 p.m.)
Review request for Default.
Description
-------
mem: Add a requestor ID to each request object.
This change adds a requestor id to each request object which can be
used identify every device in the system that is capable of issuing a request.
This is part of the way to removing the numCpus+1 stats in the cache and
replacing them with the requestor ids. This is one of a series of changes
that make way for the stats output to be changed to python.
Diffs
-----
src/arch/arm/isa.cc 223945df5510
src/arch/arm/table_walker.hh 223945df5510
src/arch/arm/table_walker.cc 223945df5510
src/arch/x86/intmessage.hh 223945df5510
src/arch/x86/pagetable_walker.hh 223945df5510
src/arch/x86/pagetable_walker.cc 223945df5510
src/cpu/base.hh 223945df5510
src/cpu/base.cc 223945df5510
src/cpu/base_dyn_inst.hh 223945df5510
src/cpu/checker/cpu.hh 223945df5510
src/cpu/checker/cpu.cc 223945df5510
src/cpu/checker/cpu_impl.hh 223945df5510
src/cpu/inorder/resources/cache_unit.cc 223945df5510
src/cpu/inorder/resources/fetch_unit.cc 223945df5510
src/cpu/inorder/resources/tlb_unit.hh 223945df5510
src/cpu/o3/fetch_impl.hh 223945df5510
src/cpu/simple/atomic.cc 223945df5510
src/cpu/simple/base.cc 223945df5510
src/cpu/simple/timing.cc 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.hh 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.cc 223945df5510
src/cpu/testers/directedtest/InvalidateGenerator.cc 223945df5510
src/cpu/testers/directedtest/RubyDirectedTester.py 223945df5510
src/cpu/testers/directedtest/SeriesRequestGenerator.cc 223945df5510
src/cpu/testers/memtest/MemTest.py 223945df5510
src/cpu/testers/memtest/memtest.hh 223945df5510
src/cpu/testers/memtest/memtest.cc 223945df5510
src/cpu/testers/networktest/NetworkTest.py 223945df5510
src/cpu/testers/networktest/networktest.hh 223945df5510
src/cpu/testers/networktest/networktest.cc 223945df5510
src/cpu/testers/rubytest/Check.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.hh 223945df5510
src/cpu/testers/rubytest/RubyTester.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.py 223945df5510
src/dev/io_device.hh 223945df5510
src/dev/io_device.cc 223945df5510
src/mem/cache/cache_impl.hh 223945df5510
src/mem/cache/prefetch/Prefetcher.py PRE-CREATION
src/mem/cache/prefetch/base.hh 223945df5510
src/mem/cache/prefetch/base.cc 223945df5510
src/mem/cache/tags/iic.cc 223945df5510
src/mem/port.cc 223945df5510
src/mem/request.hh 223945df5510
src/mem/ruby/recorder/CacheRecorder.cc 223945df5510
src/sim/system.hh 223945df5510
src/sim/system.cc 223945df5510
Diff: http://reviews.gem5.org/r/1026/diff/diff
Testing
-------
no stats changes on regression tests
Thanks,
Ali Saidi
Ali Saidi
2012-02-06 20:40:59 UTC
Permalink
Post by Ali Saidi
src/arch/arm/table_walker.hh, line 345
<http://reviews.gem5.org/r/1026/diff/1/?file=22927#file22927line345>
Seems like a typedef for RequestID might be in order. I doubt that it really needs to be a uint32_t. Also, might having a -1 value be useful?
I want absolutely no option of not setting a requester id and therefor having a value like -1. That is what lead to the mess that this hopes to fix. All requests must have a requester id and thus no magic around indexing by -1 or something need apply. The typedef is reasonable.
Post by Ali Saidi
src/arch/x86/pagetable_walker.hh, line 195
<http://reviews.gem5.org/r/1026/diff/1/?file=22930#file22930line195>
shouldn't this be a static_cast? We should always know the type. (It could be a safe_cast if you like)
yup
Post by Ali Saidi
src/cpu/testers/rubytest/Check.cc, line 106
<http://reviews.gem5.org/r/1026/diff/1/?file=22956#file22956line106>
What's this?
a problem that needs to be fixed
Post by Ali Saidi
src/mem/request.hh, line 196
<http://reviews.gem5.org/r/1026/diff/1/?file=22968#file22968line196>
These changes are somewhat dangerous because of type conversions. A simple wrapper class for the RequestID would be zero code overhead, yet it would prevent accidents by preventing implicit casts and strange bugs. Just a thought.
i thought of that, however I was very careful and checked that I didn't perturb anything with the regressions. ohh how I wish c++ supported kwargs at least for readability/verification.


- Ali


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1026/#review2083
-----------------------------------------------------------
Post by Ali Saidi
-----------------------------------------------------------
http://reviews.gem5.org/r/1026/
-----------------------------------------------------------
(Updated Feb. 4, 2012, 12:29 p.m.)
Review request for Default.
Description
-------
mem: Add a requestor ID to each request object.
This change adds a requestor id to each request object which can be
used identify every device in the system that is capable of issuing a request.
This is part of the way to removing the numCpus+1 stats in the cache and
replacing them with the requestor ids. This is one of a series of changes
that make way for the stats output to be changed to python.
Diffs
-----
src/arch/arm/isa.cc 223945df5510
src/arch/arm/table_walker.hh 223945df5510
src/arch/arm/table_walker.cc 223945df5510
src/arch/x86/intmessage.hh 223945df5510
src/arch/x86/pagetable_walker.hh 223945df5510
src/arch/x86/pagetable_walker.cc 223945df5510
src/cpu/base.hh 223945df5510
src/cpu/base.cc 223945df5510
src/cpu/base_dyn_inst.hh 223945df5510
src/cpu/checker/cpu.hh 223945df5510
src/cpu/checker/cpu.cc 223945df5510
src/cpu/checker/cpu_impl.hh 223945df5510
src/cpu/inorder/resources/cache_unit.cc 223945df5510
src/cpu/inorder/resources/fetch_unit.cc 223945df5510
src/cpu/inorder/resources/tlb_unit.hh 223945df5510
src/cpu/o3/fetch_impl.hh 223945df5510
src/cpu/simple/atomic.cc 223945df5510
src/cpu/simple/base.cc 223945df5510
src/cpu/simple/timing.cc 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.hh 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.cc 223945df5510
src/cpu/testers/directedtest/InvalidateGenerator.cc 223945df5510
src/cpu/testers/directedtest/RubyDirectedTester.py 223945df5510
src/cpu/testers/directedtest/SeriesRequestGenerator.cc 223945df5510
src/cpu/testers/memtest/MemTest.py 223945df5510
src/cpu/testers/memtest/memtest.hh 223945df5510
src/cpu/testers/memtest/memtest.cc 223945df5510
src/cpu/testers/networktest/NetworkTest.py 223945df5510
src/cpu/testers/networktest/networktest.hh 223945df5510
src/cpu/testers/networktest/networktest.cc 223945df5510
src/cpu/testers/rubytest/Check.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.hh 223945df5510
src/cpu/testers/rubytest/RubyTester.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.py 223945df5510
src/dev/io_device.hh 223945df5510
src/dev/io_device.cc 223945df5510
src/mem/cache/cache_impl.hh 223945df5510
src/mem/cache/prefetch/Prefetcher.py PRE-CREATION
src/mem/cache/prefetch/base.hh 223945df5510
src/mem/cache/prefetch/base.cc 223945df5510
src/mem/cache/tags/iic.cc 223945df5510
src/mem/port.cc 223945df5510
src/mem/request.hh 223945df5510
src/mem/ruby/recorder/CacheRecorder.cc 223945df5510
src/sim/system.hh 223945df5510
src/sim/system.cc 223945df5510
Diff: http://reviews.gem5.org/r/1026/diff/diff
Testing
-------
no stats changes on regression tests
Thanks,
Ali Saidi
nathan binkert
2012-02-06 21:19:52 UTC
Permalink
Post by Ali Saidi
i thought of that, however I was very careful and checked
that I didn't perturb anything with the regressions. ohh how
I wish c++ supported kwargs at least for readability/verification.
Honestly, we could do it if you want. Basically, if each field had a
specific type like Address, RequestID, Command, etc, then we could
make this work:

Request(Address(addr), Command(cmd), RequestID(rid)) and then I'm
pretty sure that we can come up with template magic that will do the
right thing.

But, do you want to see the compiler error? :)

Nate
Ali Saidi
2012-02-07 20:00:08 UTC
Permalink
Post by Ali Saidi
src/mem/request.hh, line 196
<http://reviews.gem5.org/r/1026/diff/1/?file=22968#file22968line196>
These changes are somewhat dangerous because of type conversions. A simple wrapper class for the RequestID would be zero code overhead, yet it would prevent accidents by preventing implicit casts and strange bugs. Just a thought.
i thought of that, however I was very careful and checked that I didn't perturb anything with the regressions. ohh how I wish c++ supported kwargs at least for readability/verification.
You mean something like:
struct RequestID
{
int rid;
explicit RequestID(int _rid) : rid(_rid) {}
};
?

Anyone have any comments?


- Ali


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1026/#review2083
-----------------------------------------------------------
Post by Ali Saidi
-----------------------------------------------------------
http://reviews.gem5.org/r/1026/
-----------------------------------------------------------
(Updated Feb. 4, 2012, 12:29 p.m.)
Review request for Default.
Description
-------
mem: Add a requestor ID to each request object.
This change adds a requestor id to each request object which can be
used identify every device in the system that is capable of issuing a request.
This is part of the way to removing the numCpus+1 stats in the cache and
replacing them with the requestor ids. This is one of a series of changes
that make way for the stats output to be changed to python.
Diffs
-----
src/arch/arm/isa.cc 223945df5510
src/arch/arm/table_walker.hh 223945df5510
src/arch/arm/table_walker.cc 223945df5510
src/arch/x86/intmessage.hh 223945df5510
src/arch/x86/pagetable_walker.hh 223945df5510
src/arch/x86/pagetable_walker.cc 223945df5510
src/cpu/base.hh 223945df5510
src/cpu/base.cc 223945df5510
src/cpu/base_dyn_inst.hh 223945df5510
src/cpu/checker/cpu.hh 223945df5510
src/cpu/checker/cpu.cc 223945df5510
src/cpu/checker/cpu_impl.hh 223945df5510
src/cpu/inorder/resources/cache_unit.cc 223945df5510
src/cpu/inorder/resources/fetch_unit.cc 223945df5510
src/cpu/inorder/resources/tlb_unit.hh 223945df5510
src/cpu/o3/fetch_impl.hh 223945df5510
src/cpu/simple/atomic.cc 223945df5510
src/cpu/simple/base.cc 223945df5510
src/cpu/simple/timing.cc 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.hh 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.cc 223945df5510
src/cpu/testers/directedtest/InvalidateGenerator.cc 223945df5510
src/cpu/testers/directedtest/RubyDirectedTester.py 223945df5510
src/cpu/testers/directedtest/SeriesRequestGenerator.cc 223945df5510
src/cpu/testers/memtest/MemTest.py 223945df5510
src/cpu/testers/memtest/memtest.hh 223945df5510
src/cpu/testers/memtest/memtest.cc 223945df5510
src/cpu/testers/networktest/NetworkTest.py 223945df5510
src/cpu/testers/networktest/networktest.hh 223945df5510
src/cpu/testers/networktest/networktest.cc 223945df5510
src/cpu/testers/rubytest/Check.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.hh 223945df5510
src/cpu/testers/rubytest/RubyTester.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.py 223945df5510
src/dev/io_device.hh 223945df5510
src/dev/io_device.cc 223945df5510
src/mem/cache/cache_impl.hh 223945df5510
src/mem/cache/prefetch/Prefetcher.py PRE-CREATION
src/mem/cache/prefetch/base.hh 223945df5510
src/mem/cache/prefetch/base.cc 223945df5510
src/mem/cache/tags/iic.cc 223945df5510
src/mem/port.cc 223945df5510
src/mem/request.hh 223945df5510
src/mem/ruby/recorder/CacheRecorder.cc 223945df5510
src/sim/system.hh 223945df5510
src/sim/system.cc 223945df5510
Diff: http://reviews.gem5.org/r/1026/diff/diff
Testing
-------
no stats changes on regression tests
Thanks,
Ali Saidi
nathan binkert
2012-02-07 20:14:45 UTC
Permalink
Post by Ali Saidi
struct RequestID
{
int rid;
explicit RequestID(int _rid) : rid(_rid) {}
};
?
Anyone have any comments?
Exactly. If we wanted to do this in more places, we could simply
create a wrapper template object.  You can pass around RequestID
objects without any extra overhead, but creating them and getting
their value requires a tiny bit of work. That work does make prevent
casting and will generally cause compiler errors when you do the wrong
thing.

template <typename T>
class TypeWrapper
{
private:
T _value;
public:
explicit TypeWrapper(const T &v) : _value(v) {}
const T &get() const { return _value; }
void set(const T& val) { _value = val; }
const TypeWrapper<T> &operator=(const TypeWrapper<T> &other) {
_value = other._value; return *this; }
};

Then you could simply do this:
typedef TypeWrapper<uint16_t> RequestID;

Another option for implementation (that requires fewer code changes
because it allows implicit conversion to the basic type and it allows
the use of operator=, but does in fact provide a bit more safety than
a basic type because it won't implicitly cast a basic type to the
wrapped type):

template <typename T>
class TypeWrapper
{
private:
T _value;
public:
explicit TypeWrapper(const T &v) : _value(v) {}
operator const T() const { return _value; }
const TypeWrapper<T> &operator=(const T &value) { _value = value;
return *this; }
const TypeWrapper<T> &operator=(const TypeWrapper<T> &other) {
_value = other._value; return *this; }
};


  Nate
Steve Reinhardt
2012-02-07 20:49:05 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1026/#review2091
-----------------------------------------------------------


In general this looks great. I just have a couple of minor concerns:
1. To me "RequestID" would be an ID for a specific request, where "RequesterID" is what you're doing here.
a. So if you're going to make a typedef or struct I'd want it named the latter.
b. Variables, arguments, and functions with "reqId" in the name are ambiguous about whether they're a RequestID or a RequesterID, and I can see that leading to confusion. Unfortunately I don't have any great ideas here. Spelling out requesterID all the time is pretty verbose. What about changing the name to masterID?
2. Is there a way to figure out what ID goes with what object and record that somewhere in the stats file or config.ini, or to assign the IDs a priori in python? I'm concerned that you'll see that requestor 17 is causing problems and have no idea what that is. getReqName() provides a basic hook here but I don't see that it gets called from anywhere.


src/sim/system.cc
<http://reviews.gem5.org/r/1026/#comment2583>

Is there (or should there be) a flag that gets set when stats are enabled?


- Steve Reinhardt
Post by Ali Saidi
-----------------------------------------------------------
http://reviews.gem5.org/r/1026/
-----------------------------------------------------------
(Updated Feb. 4, 2012, 12:29 p.m.)
Review request for Default.
Description
-------
mem: Add a requestor ID to each request object.
This change adds a requestor id to each request object which can be
used identify every device in the system that is capable of issuing a request.
This is part of the way to removing the numCpus+1 stats in the cache and
replacing them with the requestor ids. This is one of a series of changes
that make way for the stats output to be changed to python.
Diffs
-----
src/arch/arm/isa.cc 223945df5510
src/arch/arm/table_walker.hh 223945df5510
src/arch/arm/table_walker.cc 223945df5510
src/arch/x86/intmessage.hh 223945df5510
src/arch/x86/pagetable_walker.hh 223945df5510
src/arch/x86/pagetable_walker.cc 223945df5510
src/cpu/base.hh 223945df5510
src/cpu/base.cc 223945df5510
src/cpu/base_dyn_inst.hh 223945df5510
src/cpu/checker/cpu.hh 223945df5510
src/cpu/checker/cpu.cc 223945df5510
src/cpu/checker/cpu_impl.hh 223945df5510
src/cpu/inorder/resources/cache_unit.cc 223945df5510
src/cpu/inorder/resources/fetch_unit.cc 223945df5510
src/cpu/inorder/resources/tlb_unit.hh 223945df5510
src/cpu/o3/fetch_impl.hh 223945df5510
src/cpu/simple/atomic.cc 223945df5510
src/cpu/simple/base.cc 223945df5510
src/cpu/simple/timing.cc 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.hh 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.cc 223945df5510
src/cpu/testers/directedtest/InvalidateGenerator.cc 223945df5510
src/cpu/testers/directedtest/RubyDirectedTester.py 223945df5510
src/cpu/testers/directedtest/SeriesRequestGenerator.cc 223945df5510
src/cpu/testers/memtest/MemTest.py 223945df5510
src/cpu/testers/memtest/memtest.hh 223945df5510
src/cpu/testers/memtest/memtest.cc 223945df5510
src/cpu/testers/networktest/NetworkTest.py 223945df5510
src/cpu/testers/networktest/networktest.hh 223945df5510
src/cpu/testers/networktest/networktest.cc 223945df5510
src/cpu/testers/rubytest/Check.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.hh 223945df5510
src/cpu/testers/rubytest/RubyTester.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.py 223945df5510
src/dev/io_device.hh 223945df5510
src/dev/io_device.cc 223945df5510
src/mem/cache/cache_impl.hh 223945df5510
src/mem/cache/prefetch/Prefetcher.py PRE-CREATION
src/mem/cache/prefetch/base.hh 223945df5510
src/mem/cache/prefetch/base.cc 223945df5510
src/mem/cache/tags/iic.cc 223945df5510
src/mem/port.cc 223945df5510
src/mem/request.hh 223945df5510
src/mem/ruby/recorder/CacheRecorder.cc 223945df5510
src/sim/system.hh 223945df5510
src/sim/system.cc 223945df5510
Diff: http://reviews.gem5.org/r/1026/diff/diff
Testing
-------
no stats changes on regression tests
Thanks,
Ali Saidi
Ali Saidi
2012-02-07 22:58:38 UTC
Permalink
Post by Ali Saidi
Post by Steve Reinhardt
1. To me "RequestID" would be an ID for a specific request, where "RequesterID" is what you're doing here.
a. So if you're going to make a typedef or struct I'd want it named the latter.
b. Variables, arguments, and functions with "reqId" in the name are ambiguous about whether they're a RequestID or a RequesterID, and I can see that leading to confusion. Unfortunately I don't have any great ideas here. Spelling out requesterID all the time is pretty verbose. What about changing the name to masterID?
2. Is there a way to figure out what ID goes with what object and record that somewhere in the stats file or config.ini, or to assign the IDs a priori in python? I'm concerned that you'll see that requestor 17 is causing problems and have no idea what that is. getReqName() provides a basic hook here but I don't see that it gets called from anywhere.
Andreas requested masterId as well, so i guess I'll go for that.

In this change getReqName() isn't called anywhere, however it's used in the next patch to name the stat array indexes.
What is now
mshr_misses::0
mshr_misses::1
...

will instead become:
mshr_misses::cpu_dside
mshr_misses::cpu_iside
...
Post by Ali Saidi
Post by Steve Reinhardt
src/sim/system.cc, line 420
<http://reviews.gem5.org/r/1026/diff/1/?file=22971#file22971line420>
Is there (or should there be) a flag that gets set when stats are enabled?
i need to find the appropriate incantation in swig to add it. If you happen to know it, that would be great.


- Ali


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1026/#review2091
-----------------------------------------------------------
Post by Ali Saidi
-----------------------------------------------------------
http://reviews.gem5.org/r/1026/
-----------------------------------------------------------
(Updated Feb. 4, 2012, 12:29 p.m.)
Review request for Default.
Description
-------
mem: Add a requestor ID to each request object.
This change adds a requestor id to each request object which can be
used identify every device in the system that is capable of issuing a request.
This is part of the way to removing the numCpus+1 stats in the cache and
replacing them with the requestor ids. This is one of a series of changes
that make way for the stats output to be changed to python.
Diffs
-----
src/arch/arm/isa.cc 223945df5510
src/arch/arm/table_walker.hh 223945df5510
src/arch/arm/table_walker.cc 223945df5510
src/arch/x86/intmessage.hh 223945df5510
src/arch/x86/pagetable_walker.hh 223945df5510
src/arch/x86/pagetable_walker.cc 223945df5510
src/cpu/base.hh 223945df5510
src/cpu/base.cc 223945df5510
src/cpu/base_dyn_inst.hh 223945df5510
src/cpu/checker/cpu.hh 223945df5510
src/cpu/checker/cpu.cc 223945df5510
src/cpu/checker/cpu_impl.hh 223945df5510
src/cpu/inorder/resources/cache_unit.cc 223945df5510
src/cpu/inorder/resources/fetch_unit.cc 223945df5510
src/cpu/inorder/resources/tlb_unit.hh 223945df5510
src/cpu/o3/fetch_impl.hh 223945df5510
src/cpu/simple/atomic.cc 223945df5510
src/cpu/simple/base.cc 223945df5510
src/cpu/simple/timing.cc 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.hh 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.cc 223945df5510
src/cpu/testers/directedtest/InvalidateGenerator.cc 223945df5510
src/cpu/testers/directedtest/RubyDirectedTester.py 223945df5510
src/cpu/testers/directedtest/SeriesRequestGenerator.cc 223945df5510
src/cpu/testers/memtest/MemTest.py 223945df5510
src/cpu/testers/memtest/memtest.hh 223945df5510
src/cpu/testers/memtest/memtest.cc 223945df5510
src/cpu/testers/networktest/NetworkTest.py 223945df5510
src/cpu/testers/networktest/networktest.hh 223945df5510
src/cpu/testers/networktest/networktest.cc 223945df5510
src/cpu/testers/rubytest/Check.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.hh 223945df5510
src/cpu/testers/rubytest/RubyTester.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.py 223945df5510
src/dev/io_device.hh 223945df5510
src/dev/io_device.cc 223945df5510
src/mem/cache/cache_impl.hh 223945df5510
src/mem/cache/prefetch/Prefetcher.py PRE-CREATION
src/mem/cache/prefetch/base.hh 223945df5510
src/mem/cache/prefetch/base.cc 223945df5510
src/mem/cache/tags/iic.cc 223945df5510
src/mem/port.cc 223945df5510
src/mem/request.hh 223945df5510
src/mem/ruby/recorder/CacheRecorder.cc 223945df5510
src/sim/system.hh 223945df5510
src/sim/system.cc 223945df5510
Diff: http://reviews.gem5.org/r/1026/diff/diff
Testing
-------
no stats changes on regression tests
Thanks,
Ali Saidi
Andreas Hansson
2012-02-09 10:15:58 UTC
Permalink
Post by Ali Saidi
Post by Steve Reinhardt
1. To me "RequestID" would be an ID for a specific request, where "RequesterID" is what you're doing here.
a. So if you're going to make a typedef or struct I'd want it named the latter.
b. Variables, arguments, and functions with "reqId" in the name are ambiguous about whether they're a RequestID or a RequesterID, and I can see that leading to confusion. Unfortunately I don't have any great ideas here. Spelling out requesterID all the time is pretty verbose. What about changing the name to masterID?
2. Is there a way to figure out what ID goes with what object and record that somewhere in the stats file or config.ini, or to assign the IDs a priori in python? I'm concerned that you'll see that requestor 17 is causing problems and have no idea what that is. getReqName() provides a basic hook here but I don't see that it gets called from anywhere.
Andreas requested masterId as well, so i guess I'll go for that.
In this change getReqName() isn't called anywhere, however it's used in the next patch to name the stat array indexes.
What is now
mshr_misses::0
mshr_misses::1
...
mshr_misses::cpu_dside
mshr_misses::cpu_iside
...
Indeed, I would prefer masterId as it identifies the MasterPort where the transaction request is created. We should also clearly state if/how it is allowed to be changed, i.e. if it is forwarded by a bridge, must it always retain its original masterId, or is the bridge (acting as a master on another bus) allowed to change it?

A more long term name change that I would like to suggest is to rename the Request class Transaction, since it lives over the span of both a request, forwarded request, forwarded response, and response etc (irrespective of the length of the message chain). A Transaction is then persistent over a number of phases, and comprises a number of messages that are exchanged. Ultimately, the current Packet class would be split into a "transaction layer" Message class (one for each phase of a transaction), and an interconnect "transport layer" with a Packet class that wraps messages and adds interconnect specific information. To start with a Packet would always correspond 1-1 to a Message, but that could change over time for other (future) interconnects. I'm going slightly astray here, so please pro
vide any feedback in a separate thread.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1026/#review2091
-----------------------------------------------------------
Post by Ali Saidi
-----------------------------------------------------------
http://reviews.gem5.org/r/1026/
-----------------------------------------------------------
(Updated Feb. 4, 2012, 12:29 p.m.)
Review request for Default.
Description
-------
mem: Add a requestor ID to each request object.
This change adds a requestor id to each request object which can be
used identify every device in the system that is capable of issuing a request.
This is part of the way to removing the numCpus+1 stats in the cache and
replacing them with the requestor ids. This is one of a series of changes
that make way for the stats output to be changed to python.
Diffs
-----
src/arch/arm/isa.cc 223945df5510
src/arch/arm/table_walker.hh 223945df5510
src/arch/arm/table_walker.cc 223945df5510
src/arch/x86/intmessage.hh 223945df5510
src/arch/x86/pagetable_walker.hh 223945df5510
src/arch/x86/pagetable_walker.cc 223945df5510
src/cpu/base.hh 223945df5510
src/cpu/base.cc 223945df5510
src/cpu/base_dyn_inst.hh 223945df5510
src/cpu/checker/cpu.hh 223945df5510
src/cpu/checker/cpu.cc 223945df5510
src/cpu/checker/cpu_impl.hh 223945df5510
src/cpu/inorder/resources/cache_unit.cc 223945df5510
src/cpu/inorder/resources/fetch_unit.cc 223945df5510
src/cpu/inorder/resources/tlb_unit.hh 223945df5510
src/cpu/o3/fetch_impl.hh 223945df5510
src/cpu/simple/atomic.cc 223945df5510
src/cpu/simple/base.cc 223945df5510
src/cpu/simple/timing.cc 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.hh 223945df5510
src/cpu/testers/directedtest/DirectedGenerator.cc 223945df5510
src/cpu/testers/directedtest/InvalidateGenerator.cc 223945df5510
src/cpu/testers/directedtest/RubyDirectedTester.py 223945df5510
src/cpu/testers/directedtest/SeriesRequestGenerator.cc 223945df5510
src/cpu/testers/memtest/MemTest.py 223945df5510
src/cpu/testers/memtest/memtest.hh 223945df5510
src/cpu/testers/memtest/memtest.cc 223945df5510
src/cpu/testers/networktest/NetworkTest.py 223945df5510
src/cpu/testers/networktest/networktest.hh 223945df5510
src/cpu/testers/networktest/networktest.cc 223945df5510
src/cpu/testers/rubytest/Check.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.hh 223945df5510
src/cpu/testers/rubytest/RubyTester.cc 223945df5510
src/cpu/testers/rubytest/RubyTester.py 223945df5510
src/dev/io_device.hh 223945df5510
src/dev/io_device.cc 223945df5510
src/mem/cache/cache_impl.hh 223945df5510
src/mem/cache/prefetch/Prefetcher.py PRE-CREATION
src/mem/cache/prefetch/base.hh 223945df5510
src/mem/cache/prefetch/base.cc 223945df5510
src/mem/cache/tags/iic.cc 223945df5510
src/mem/port.cc 223945df5510
src/mem/request.hh 223945df5510
src/mem/ruby/recorder/CacheRecorder.cc 223945df5510
src/sim/system.hh 223945df5510
src/sim/system.cc 223945df5510
Diff: http://reviews.gem5.org/r/1026/diff/diff
Testing
-------
no stats changes on regression tests
Thanks,
Ali Saidi
Loading...