src/cpu/testers/rubytest/RubyTester.hh (Diff revision 1)

Why not call these getInstPort and getDataPort like we do for the actual CPU models, or is that not what it is supposed to model?

Show all issues

1. Brad Beckmann April 5, 2012, 5:05 a.m. (April 5, 2012, 5:05 a.m.)

   Yeah, it is a little confusing, but inst and data ports are not equivalent to the read and write ports.  From the tester's perspective, it only needs to know whether a port supports reads (checks) or writes (actions).  Meanwhile, the protocol controllers have data ports (support read and writes) or inst ports (support only reads).

2. Steve Reinhardt April 5, 2012, 5:31 a.m. (April 5, 2012, 5:31 a.m.)

   So looking through the code, the RubyTester wants a list of ports it can issue reads to, and a separate list of ports it can issue writes to (and the key contribution of this patch is to have two separate lists and not a single merged read/write list).  Dcache ports go on both lists, but Icache ports go on the read list only.  IMO the problems here are (1) there's no comment in the code that really explains that this is what is going on at this level, and (2) the term "Cpu" is still embedded in a number of places (like getCpuReadPort) where maybe it doesn't make sense anymore.
   
   On that second point, it looks to me like the term "cpu port" was used because the tester could be thought of as emulating multiple CPUs, and each port corresponded to one emulated CPU.  Now we have extended this analogy to where arguably we're still emulating multiple CPUs, some of which only issue reads and some of which only issue writes, and which then go through a mapping to get to the right types of ports, but it's not a realistic model anymore.  Thus Andreas's confusion that we're actually trying to access the ports on a particular CPU, in which case "read port" and "write port" don't make sense, but "data port" and "inst port" do.  It might be a tedious and semantically meaningless thing to do at this point, but I think going through and eliminating "Cpu" from all the variable and method names in the tester where it doesn't actually mean anything (which I would guess is most or all of them) would make things clearer to future generations.  That could be a separate patch though.

3. Brad Beckmann April 5, 2012, 7:29 a.m. (April 5, 2012, 7:29 a.m.)

   I agree that I should have had a better comment in the RubyTester constructor where that mapping logic exists (see latest diff).  I thought I made that point in my previous patch, but instead I only discussed the vector index assumptions.
   
   I believe the term "cpu port" should still be used because it differentiates the cpu ports from dma ports.  In ruby configuration, cpu and dma ports are enumerated differently and the ruby tester currently only supports cpu ports.  I think this is an important distinction, so I would prefer to keep the current names.

4. Steve Reinhardt April 5, 2012, 7:44 a.m. (April 5, 2012, 7:44 a.m.)

   Thanks for the update and the clarification.  I think explicitly saying something in the comment along the lines of "data ports go on both lists, but inst ports go only on the read list" would help a lot in understanding the distinction.  That's the insight that made it clear to me.
   
   I agree that the "Cpu" term is useful to track the CPU vs DMA port distinction (which obviously didn't occur to me previously).  How about changing the names more subtly to something like "getReadableCpuPort" and "getWritableCpuPort", or something similar?  I think that naming makes it clearer that we're talking about a CpuPort that's readable or writable, as opposed to "the CPU's write port" which is something that  conceptually could but doesn't actually exist.

I would suggest inst/data rather than read/write throughout to be consistent with the gem5 CPU models.

src/cpu/testers/rubytest/RubyTester.cc (Diff revision 3)

This seems to be wrong if a data port is in both read and write. Overall, I would prefer an organisation of instPorts and dataPorts, and then use the getRead/Write Port to do the "magic" mapping into these two.

Show all issues

1. Brad Beckmann April 6, 2012, 3:14 a.m. (April 6, 2012, 3:14 a.m.)

   I think we are "splitting hairs" here and this is getting into more a philosophical discussion.  I know I used the term magic before, but really there isn't anything really complicated going on here.  It is simply the tester cares about read and write ports, but individual Ruby controllers can be constructed to manage inst or data caches.

2. Andreas Hansson April 6, 2012, 3:26 a.m. (April 6, 2012, 3:26 a.m.)

   As far as I can tell, delete is called twice on a data port pointer since it is in both read and write ports. Or am I missing something?

3. Brad Beckmann April 6, 2012, 6:35 a.m. (April 6, 2012, 6:35 a.m.)

   Good catch!  Yep, that is a bug.  Good think gem5 doesn't call destructors.  :)

src/cpu/testers/rubytest/RubyTester.cc (Diff revision 3)

May as well made the index a size_t to please the STL array lookup and comparison with size().

Show all issues

1. Brad Beckmann April 6, 2012, 3:14 a.m. (April 6, 2012, 3:14 a.m.)

   One could agrue there are several, possibly hundreds, of situations where we should use size_t instead of ints.  If we decide that is important, I feel like that is something that needs be done in a larger separate patch.

2. Andreas Hansson April 6, 2012, 3:26 a.m. (April 6, 2012, 3:26 a.m.)

   Absolutely fine...it was just a suggestion, have to start somewhere :)

Thanks for the changes!