----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1827/#review4257 -----------------------------------------------------------
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 <http://reviews.gem5.org/r/1827/#comment4039> 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. src/cpu/pred/tournament.cc <http://reviews.gem5.org/r/1827/#comment4040> 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. - Erik Tomusk On April 18, 2013, 7:26 a.m., Anthony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1827/ > ----------------------------------------------------------- > > (Updated April 18, 2013, 7:26 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9638:213065dd02d9 > --------------------------- > cpu: remove local/globalHistoryBits params from branch pred > > having separate params for the local/globalHistoryBits and the > local/globalPredictorSize can lead to inconsistencies if they > are not carefully set. this patch dervies the number of bits > necessary to index into the local/global predictors based on > their size. > > the value of the localHistoryTableSize for the ARM O3 CPU has been > increased to 1024 from 64, which is more accurate for an A15 based > on some correlation against A15 hardware. > > > Diffs > ----- > > configs/common/O3_ARM_v7a.py c22628fa25646077ed4f18a811f348b47a64bd5d > src/cpu/pred/BranchPredictor.py c22628fa25646077ed4f18a811f348b47a64bd5d > src/cpu/pred/tournament.cc c22628fa25646077ed4f18a811f348b47a64bd5d > > Diff: http://reviews.gem5.org/r/1827/diff/ > > > Testing > ------- > > > Thanks, > > Anthony Gutierrez > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
