> On April 19, 2013, 5:55 a.m., Erik Tomusk wrote: > > src/cpu/pred/tournament.cc, line 52 > > <http://reviews.gem5.org/r/1827/diff/3/?file=35140#file35140line52> > > > > 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.
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. > On April 19, 2013, 5:55 a.m., Erik Tomusk wrote: > > src/cpu/pred/tournament.cc, line 55 > > <http://reviews.gem5.org/r/1827/diff/3/?file=35140#file35140line55> > > > > 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. 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. - Anthony ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1827/#review4257 ----------------------------------------------------------- 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
