A few thing below. Overall, could you please add more comments? No need to write the entire TAGE paper in the comments, but a little high-level information would help new people coming to this code a lot.

src/cpu/o3/O3CPU.py (Diff revision 1)

I'm not sure we want to make the TAGE predictor the default. Are there any products that use something like the TAGE predictor?
Alternatively, if the tournamentBP is much worse than current products, I can see the argument for make the TAGE predictor the default.

Show all issues

src/cpu/pred/BranchPredictor.py (Diff revision 1)

Is the global history really 128 megabytes?
I'm not very familiar with the TAGE predictor. Does a real implementation somehow reduce this size and you're making a simplifying assumtion here?

Show all issues

src/cpu/pred/ltage.hh (Diff revision 1)

Can you add some comments on all of these functions? Doxygen comments would be best.

Show all issues

src/cpu/pred/ltage.cc (Diff revision 1)

I'm not sure if you're really using "histBufferSize" as a "MemorySize". This seems to just be a count that defaults to 2**27. Is this really a MemorySize?

Show all issues

Ship It!

Thanks for the updates. I'm still not clear on the histBufferSize (as detailed below).

src/cpu/pred/BranchPredictor.py (Diff revision 2)

Comment is now wrong.
Also, does it make more sense to call this histBufferEntries (or histBufferBits) and make the default 221? (or would it be 221*8? <-- this is the confusion.) 
I think it would be more clear to use bits or entries, personally. But if the branch predictor papers usually talk about the history buffer in terms of bytes instead of bits, I guess the current version makes more sense.

Show all issues

1. Nathanael Premillieu Nov. 23, 2016, 7:31 a.m. (Nov. 23, 2016, 7:31 a.m.)

   This big buffer is just a software structure and it does not represent a hardware one.
   
   It is just done like this to simplify the management of the branch history and avoid copying those bits too often.
   
   The modeled hardware history is implemented as a sliding window on this big buffer.

2. Arthur Perais Nov. 23, 2016, 7:39 a.m. (Nov. 23, 2016, 7:39 a.m.)

   In effect, the predictor will create an array of 2**21 entries, each entry containing an uint8_t, so the buffer is indeed 2MB, even though it contains 2Mbits of history information. As Nathanael pointed out, this is a trick to avoid using a linked-list/deque which is more costly. The buffer is much bigger than the actual history size to remove bound checks and wrap-around adjustment that would be necessary with a circular buffer in *most* places where it is accessed, at the cost of copying the whole history from the beginning to the end of the buffer when the buffer is filled (so it has to be big enough to amortize the cost of the copy).
   
   If you really think it should be changed to Param.Unsigned, I'll change it. I'll wait for some input before posting the version with the correct comment (my bad).

3. Jason Lowe-Power Dec. 15, 2016, 11:44 a.m. (Dec. 15, 2016, 11:44 a.m.)

   Yeah, I think it's much better to use an unsigned (int). Otherwise it's not clear whether this is the number of bits, or the number of entries, or something else.

Thanks

src/cpu/pred/ltage.hh (Diff revision 2)

Why not use a vector<bool>? This is what it's actually representing, correct?
I would argue for using the datatype that makes it most clear to future people reading this code.

Show all issues

1. Arthur Perais Dec. 16, 2016, 6:24 a.m. (Dec. 16, 2016, 6:24 a.m.)

   This is true, but unfortunately I really don't have time to refactor right now. I will leave a comment in the code (e.g., TODO) for future reference.

Ship It!