Thanks for taking the time to do this Tony!

It's pretty much looks great, however, the style guide says that member variables should be mixed case and not contain underscores (e.g. instructionsExecuted instead of instructions_executed). However, we do tends to name the stats output at instructions_executed which is a bit annoying. Unless someone feels strongly I think it would be best to stick to the style guide.

Hi Tony, 

Could you check that running the regressions without your change and with your change the same values appear in stats.txt with the appropriate regexes applied to the new file? 

We need to be really certain that we haven't accidentally recorded something incorrectly. 

Thanks,
Ali

src/cpu/inorder/cpu.cc (Diff revision 2)

Is "Operations" the right word here? It seems ambiguous.

Assuming that numSimulatedInstructions means macro-ops, I think the next derivative of that should mean "numSimulatedMicroOps". 

Although the comment clarifies this, operations sounds more like the number of Functional Unit *operations* then micro-ops in my opinion.

1. Ali Saidi Jan. 11, 2012, 5:37 a.m. (Jan. 11, 2012, 5:37 a.m.)

   I like operations, but I think it's open to debate. macro-ops and micro-ops are a bit weird. Is an instruction that isn't (cracked, micro-coded, whatever) a macro-op or a micro-op? 
   
   I think instructions should be whatever is derived from a "word" of machine code be it 1 or N instruction and then there needs to be something that is everything that has execute() called on it. This isn't really micro-ops, because you could have a signal instruction that isn't a micro-op, but it should count toward the total. ops or operations makes sense to me here. 
   
   
   
   

2. Korey Sewell Jan. 11, 2012, 7:33 a.m. (Jan. 11, 2012, 7:33 a.m.)

   Ali,
   I'm assuming that a single instruction gets counted as one operation so that the number of operations is always >= the number of instructions. Does that make sense?
   
   To try to be clearer, if you had this instruction trace:
   <start macro-op sn:1>
   <micro-op 1>
   <micro-op 2>
   <micro-op 3>
   <end macro-op sn:1>
   signal instruction
   <start macro-op sn:2>
   <micro-op 1>
   <micro-op 2>
   <micro-op 3>
   <end macro-op sn:2>
   
   Then your stats would look like:
   totalInstructions: 3
   totalOps/Operations:7
   
   Would you agree on that stat count?
   
   Using micro-op for an instruction that isn't "cracked" is fuzzy and probably just wrong so I see your contention there. 

3. Gabe Black Jan. 11, 2012, 7:54 a.m. (Jan. 11, 2012, 7:54 a.m.)

   Sequence numbers (sn:xxx) are assigned to the StaticInsts that make it down the pipe, so macroops don't have them at all and microops (and regular instructions, and speculatively executed instances of each) would each have a unique one.

4. Korey Sewell Jan. 11, 2012, 8:12 a.m. (Jan. 11, 2012, 8:12 a.m.)

   Ahhh, I see. 
   
   I think this is an issue that I was ran into with the InOrder CPU and microps where I wanted each micro-op to have a separate sub number off the micro-op such as:
   <macro-op sn:1 micro-op 1>
   <macro-op sn:1 micro-op 2>
   <macro-op sn:2 micro-op 3>
   <signal instruction sn:2>
   etc.
   That makes sense to me but that's not how it's done in any of the models so it's up to me to conform there :)
   
   As for my running example of how stats would look like, I'd like to give it another pass. 
   
   The trace would be this:
   <start macro-op>
   micro-op sn:1
   micro-op sn:2
   micro-op sn:3
   <end macro-op>
   signal instruction sn:4
   <start macro-op>
   micro-op sn:5
   micro-op sn:6
   micro-op sn:7
   <end macro-op>
   
   Would the stats be this (call it A):
   ops:7
   insts: 3 (counting a whole macro-op as an instruction)
   
   or this (call it B):
   ops: 7
   insts:1
   

Thanks, Tony, this is long overdue.

A few thoughts:

- I think "insts" and "ops" are pretty good names, with the definitions that Ali used.  However, somehow even though expanding "insts" to "instructions" seems OK, expanding "ops" to "operations" paradoxically makes things more confusing in my mind.  Since everyone is used to saying "micro-op" or "macro-op" (or "uop"), "op" has a clear lineage, but no one says "micro-operation" in practice, so "operation" sounds like a more generic term that could mean almost anything.  I would be happy to see names like totalInstructions() and numSimulatedInstructions() replaced with totalInsts() and numSimulatedInsts() so that their new counterparts can be named totalOps() and numSimulatedOps().  I don't think replacing "insts" with "instructions" actually makes the code any more readable anyway.

- I don't like losing the "committed" part of some of those stat names; on anything with speculative execution (InOrder or O3) there will be a distinction between the number of insts/ops executed and the number committed.  I assume these renames were done for consistency with other CPU models (which is good!), but it's hard to know for sure since the diff doesn't show the unchanged names and I'm too lazy to look right now...

- Do make sure that the commit message is correct when this finally gets committed!

Looks good to me... Korey? Steve?

Ship It!

src/cpu/inorder/cpu.hh (Diff revision 6)

I'm still a little confused here... it seems like either we should not rename these stats from "committed" to "executed", or the comments are wrong and should say "executed" rather than "committed".

Show all issues

1. Korey Sewell Jan. 23, 2012, 1:04 p.m. (Jan. 23, 2012, 1:04 p.m.)

   Tony/Steve/Ali,
   I'm traveling and won't be "in-place" until Wednesday.
   
   Please don't commit until I have a chance to review this further.
   
   On a first pass (Tony), make sure you are changing the correct stat because their is an ExecutionUnit that has a "executedInsts" stat that counts all ALU operations. Unless we are counting micro-ops as "executed" now I think that name should stay as committed. 
   
   The actual instructions "executed" is different than the number of "committed" due to misspeculation/interrupts/traps/etc (especially for the O3).
   
   So I'm legitimately confused.
   
   Please continue discussion but give me at least a day to comment before committing.
   
   Thanks,
   Korey

2. Tony Gutierrez Jan. 24, 2012, 1:41 a.m. (Jan. 24, 2012, 1:41 a.m.)

   Oh sorry, I meant to fix those comments to say executed. The reason I changed it to executed from committed is because the commit stage has a stat, which essentially counts the same thing, and I called that committed. I can change it back to committed, or I can change everything in the CPU to executed across the board.

3. Korey Sewell Jan. 26, 2012, 6:26 p.m. (Jan. 26, 2012, 6:26 p.m.)

   Tony,
   first off thanks for doing this. It's a tedious process and a lot of back/forth to get the names right (not to mention to update the regressions once we agree!).
   
   second, I think there should be a clear distinction between "executed" and "committed".  Executing meaning some work done on an instruction by a functional unit and committed meaning actually retired from the CPU successfully. 
   
   Keeping track of what's "executed" can be especially important for power modeling of function units.

Anyone else have issues? I'd like to get this committed as we've been kicking it around for a long while now

src/cpu/simple/base.cc (Diff revision 7)

should the simple cpu stats be committedOps and commitedInsts so they're named the same as their inorder/o3 counterparts?

Show all issues

My only concern was about some of the naming in the InOrder model, and I can't see the resolution there because reviewboard is giving errors on those specific diffs.  However, as long as that's resolved to everyone else's satisfaction, then I'm fine with it.  I agree it would be good to have this committed.

Ship It!

Any last comments before we commit this?

src/cpu/simple/base.cc (Diff revisions 7 - 8)

How does it work if both BaseCPU and InOrderCPU have separate stats called committedInsts?

Show all issues

1. Tony Gutierrez Feb. 9, 2012, 12:12 a.m. (Feb. 9, 2012, 12:12 a.m.)

   This is simple/base.cc, cpu/base.cc does not have an instruction stat. All the cpu models have the .committedInsts to represent the number of actual ISA instructions committed.

2. Steve Reinhardt Feb. 9, 2012, 12:37 a.m. (Feb. 9, 2012, 12:37 a.m.)

   Of course... my mistake, sorry about that.
   
   One could argue that the next logical step is to factor these common stats up into cpu/base.cc, but that's a separate issue.  This is clearly an improvement.  Thanks!

3. Korey Sewell Feb. 9, 2012, 12:41 a.m. (Feb. 9, 2012, 12:41 a.m.)

   Last thing Tony... after you commit it, make sure you update the regressions. 
   
   That bites everyone from time to time, so with a change like this that effects all CPU model accounting, you'll have a number (all?) of regression stats to update.
   
   I know firsthand that the "pain and suffering" of updating a bunch of regressions sometimes can be stagnating, but don't let that stop you!
   
   

Ship It!