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

Can't you just use ceilLog2() from src/base/intmath.hh?

Show all issues

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

same here

Show all issues

seems good to me.

Ship It!

Overall it looks good, with only some minor things. The only thing I'd like you to change is the keyword. If the list (http://gem5.org/Commit_Access) needs additional items we should kick off a discussion.

"The keyword should be one of the following: base, stats, sim, config, ruby, mem, arm, x86, arch, cpu, dev, scons, alpha, mips, power, sparc"

src/cpu/pred/tournament.cc (Diff revision 2)

I would suggest to move the ceilLog2 intialisation here.

If not, please initialise them to 0 here to not upset static code analysers :-)

Show all issues

src/cpu/pred/tournament.cc (Diff revision 2)

Remove the commented out line and preferably move the initialisation to the initialiser list

Show all issues

Ship It!

I'm not sure about either of these changes. See comments.

What inconsistencies do you have in mind?

I think I was the last one to dig into this bit of code, so if anything is wrong, it's probably my fault. The one confusing thing about the branch predictor is that the tournament and the local use a partially overlapping set of parameters. ATM (i.e. before this patch), localPredictorSize can be defined in the configuration because it's relevant to the local predictor, but it has no effect on the tournament predictor.

src/cpu/pred/tournament.cc (Diff revision 3)

2^localHistoryBits=localPredictorSize, so this is just a cosmetic change. When I set it up so that localHistoryBits was the input to the  class, it was because I thought the user would be more interested in setting how many bits of history they want, rather than setting the number of counters and having gem5 calculate the required number of history bits. This also preempts the user from setting a non-power-of-2 predictor size.

But if making the predictor size the input and determining the number of history bits from that makes sense to everyone else, so be it.

1. Tony Gutierrez April 18, 2013, 11:18 p.m. (April 18, 2013, 11:18 p.m.)

   This patch was inspired by an inconsistency observed in the current ARM O3 config, i.e., localHistoryBits = 6, however localPredictorSize = 1024. It seems as though the person who made this config was under the impression 2^localHistoryBits = localHistoryTableSize, which seems reasonable given the naming.

2. Tony Gutierrez April 18, 2013, 11:22 p.m. (April 18, 2013, 11:22 p.m.)

   Sorry, the localPredictorSize is set to the default, 2048, not 1024.

src/cpu/pred/tournament.cc (Diff revision 3)

This change will cause problems. globalHistoryBits sets the number of bits of global branch history kept. This number is independent from globalPredictorSize, because both the global and the choice predictor use the global history buffer. There is a test further down (~ line 115) to ensure that the globalHistoryBits is large enough, given choicePredictorSize and globalPredictorSize. Alternatively, globalHistoryBits should be max(ceilLog2(globalPredictorSize), ceilLog2(choicePredictorSize)). tournament.hh has lots of comments on what all of these variables do.

Show all issues

1. Tony Gutierrez April 18, 2013, 11:18 p.m. (April 18, 2013, 11:18 p.m.)

   As said above, the naming may be a little confusing, which can cause inconsistencies when setting these parameters. If they have some well-defined mathematical relationship, and only one parameter is really important for the user, then we should allow the user to set one and derive the other.

Are they anymore concerns/objections to this?