Jason, thanks for the feedback, I'll do my best to address everything tomorrow, but I've added some replies to your comments inline.
----- Mail original ----- > De: "Jason Lowe-Power" <[email protected]> > À: "Default" <[email protected]>, "Jason Lowe-Power" <[email protected]>, > "Arthur Perais" <[email protected]> > Envoyé: Mardi 22 Novembre 2016 19:15:03 > Objet: Re: Review Request 3743: cpu: implement L-TAGE branch predictor > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3743/ > 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. Will do. > src/cpu/o3/O3CPU.py (Diff revision 1) > def support_take_over(cls): > 142 > branchPred = Param . BranchPredictor ( TournamentBP ( numThreads = > 142 > branchPred = Param . BranchPredictor ( LTAGE ( numThreads = > 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. Well, officially no, but then again, Seznec is the only guy that has an Intel Research Impact Medal so maybe :) In addition, perceptron is known to be in AMD (Zen I think?) and Samsung (Mongoose, which is mobile-class) CPUs, and its performance is ~comparable to TAGE, so in that sense, it is reasonable to have a high performing branch predictor as the default for the out-of-order CPU. I'm not pushing for it though, totally fine to keep the tournament as default. > src/cpu/pred/BranchPredictor.py (Diff revision 1) 99 > histBufferSize = Param . MemorySize ( "128MB" , > 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? I'm not quite sure what is happening here, but 128MB is way overkill. TAGE typically tracks the past ~100 - ~1000 last outcomes. Will look into it. > src/cpu/pred/ltage.hh (Diff revision 1) 181 > int bindex ( Addr pc_in ) const ; > Can you add some comments on all of these functions? Doxygen comments would > be best. > src/cpu/pred/ltage.cc (Diff revision 1) 68 > history . globalHistory = new uint8_t [ histBufferSize ]; > 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? Shouldn't be. Will look into it as well. > - Jason Lowe-Power > On November 22nd, 2016, 2:31 p.m. UTC, Arthur Perais wrote: > Review request for Default. > By Arthur Perais. > Updated Nov. 22, 2016, 2:31 p.m. > Repository: gem5 > Description > Changeset 11707:1d085f66c4ca > cpu: implement an L-TAGE branch predictor > This patch implements an L-TAGE predictor, based on André Seznec's code > available from CBP-2 > (http://hpca23.cse.tamu.edu/taco/camino/cbp2/cbp-src/realistic-seznec.h). > The patch also changes the default branch predictor of o3 from the > tournament predictor to L-TAGE. > This patch requires patch #3727 (http://reviews.gem5.org/r/3727/) to compile. > Diffs > * src/cpu/o3/O3CPU.py (1d085f66c4ca) > * src/cpu/pred/BranchPredictor.py (1d085f66c4ca) > * src/cpu/pred/SConscript (1d085f66c4ca) > * src/cpu/pred/ltage.hh (PRE-CREATION) > * src/cpu/pred/ltage.cc (PRE-CREATION) > View Diff _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
