src/cpu/o3/fetch_impl.hh (Diff revision 2)

What is this line doing? 

Show all issues

1. Ali Saidi Jan. 31, 2012, 2:06 a.m. (Jan. 31, 2012, 2:06 a.m.)

   Is there a simulated performance difference (modulo subtle changes)? 

2. Nilay Vaish Jan. 31, 2012, 2:36 a.m. (Jan. 31, 2012, 2:36 a.m.)

   IIRC, there are three ways in which the fetch stage can generate the next
   instruction. It can use the micro code ROM, or the current macroop, or the
   available cache block. I think when the code was originally written, there
   was some assumption that in a given cycle only one of the three ways would
   be used. But it seems over time, this changed. What can happen now is that
   the fetch stage might start fetching from one of the three possible ways,
   but midway through the cycle, it might switch to some other way. One of my
   previously posted patches fixes the handling for micro code rom. Over here
   before the fetch stage decides to make use of the cache block, it should
   confirm that the PC lies with in the address range of the cache block 
   under consideration. If not, then the fetch stage should stop fetching
   instructions and access the icache.

src/cpu/o3/rob_impl.hh (Diff revision 2)

How often is this called? Would it make more sense to have a pointer to the previous instruction in every instruction that what through this (potentially 100+ element) list? 

Show all issues

1. Nilay Vaish Jan. 31, 2012, 2:36 a.m. (Jan. 31, 2012, 2:36 a.m.)

   It seems that the only place where the squashed instruction is in fetch
   stage where the current macroop is derived from it. We might want to
   change iew and commit to just send the current macroop to the fetch
   stage. Currently iew forwards only the instruction number, so that has to be
   looked up in the ROB to figure out the instruction itself. Since iew and
   commit stages have the microops, they can get the pointer to the macroop
   as well.

I do not disagree with the spirit of this patch. In fact I think I advocated for it. Concretely there are some minor style issues to straighten out, and a function that could be simplified or eliminated. I don't see anything wrong with what you're actually doing, but the squashing logic for O3 makes my head hurt and I won't pretend to understand all of it. If you've tested it thoroughly then that's all I'd be able to do in your shoes.

src/cpu/o3/fetch_impl.hh (Diff revision 2)

break on the next line please.

Show all issues

src/cpu/o3/rob.hh (Diff revision 2)

Judging by the comment, this isn't really getting the squash inst, it just happens to be used that way. What it's really doing is getting an instruction with a particular sequence number, right?

Show all issues

1. Nilay Vaish Feb. 2, 2012, 6:15 a.m. (Feb. 2, 2012, 6:15 a.m.)

   Right.

2. Gabe Black Feb. 2, 2012, 6:30 a.m. (Feb. 2, 2012, 6:30 a.m.)

   Which implies that it should be named something like "findInst" which describes it more accurately. I'm assuming you got my point, but just in case.

src/cpu/o3/rob_impl.hh (Diff revision 2)

This whole function could be replaced by (or perhaps wrap) one of the STL searching functions. If there isn't one that's a member of whatever type the instruction list is, then I'm pretty sure there's one in the generic algorithms stuff.

Unfortunately I couldn't tell you off hand what it's called or where to find it, but I'm confident it's in there because I think I've used it before. It might even use some fancy schmancy algorithm that would be faster than this for loop, but if it's a linked list likely not.

Show all issues

1. Nilay Vaish Feb. 2, 2012, 6:15 a.m. (Feb. 2, 2012, 6:15 a.m.)

   I think you are talking about the search function. It requires a comparison
   operator for dynamic instructions. I am not sure if this is necessarily less
   amount of code.

2. Gabe Black Feb. 2, 2012, 6:30 a.m. (Feb. 2, 2012, 6:30 a.m.)

   This looks like what I was thinking of:
   
   http://www.sgi.com/tech/stl/find.html

3. Nilay Vaish Feb. 2, 2012, 7:04 a.m. (Feb. 2, 2012, 7:04 a.m.)

   I don't think this would work. Here is the prototype of the function --
   InputIterator find(InputIterator first, InputIterator last, Type const &value);
   
   The function returns the first iterator i in the range [first, last) such that 
   *i == value. In our case, the equality is not being tested on the underlying
   object, but on one of its member.
   
   The find_if function solves this problem, but again does not seem like less
   amount of code.
   http://www.sgi.com/tech/stl/find_if.html

4. Gabe Black Feb. 2, 2012, 7:19 a.m. (Feb. 2, 2012, 7:19 a.m.)

   Yeah, I see your point. Never mind then.

src/cpu/o3/rob_impl.hh (Diff revision 2)

Brace at the end of the previous line please.

Show all issues

src/cpu/o3/commit_impl.hh (Diff revision 2)

It looks like we already have the DynInstPtr here in the case where the rob is not empty... in fact just looking at this code I'd say that the call to getSquashInst() below will only ever return either NULL or whatever rob->readHeadInst() returns here.  Right? 

1. Nilay Vaish Feb. 2, 2012, 6:15 a.m. (Feb. 2, 2012, 6:15 a.m.)

   Reading the code, it seems like it will always return NULL. I think neither
   of those two sequence numbers can be possibly in the ROB. I'll put an assert
   statement and test if anything other NULL is returned.

src/cpu/o3/fetch_impl.hh (Diff revision 2)

This definitely needs a comment that summarizes your response to Ali for posterity.

Show all issues