----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/929/#review1751 -----------------------------------------------------------
Without wanting to dive into the branch predictor code I'll assume this is functionally correct, especially if you've run regressions. There are some style nits which I've pointed out. I'm also assuming *this* change doesn't break X86_FS on O3, and that this can be applied separately. I'm sure you're fixing a legitimate problem, but for my information could you please explain what the problem a little more specifically? There's no need to go into the gory details, I'd just like to be able to tell in the future if I'm running into whatever this is fixing. The branch predictor was just not handling returns from calls correctly? src/cpu/o3/bpred_unit_impl.hh <http://reviews.m5sim.org/r/929/#comment2263> should be on the same line as the } src/cpu/o3/bpred_unit_impl.hh <http://reviews.m5sim.org/r/929/#comment2264> change to hist_it->usedRAS? src/cpu/o3/bpred_unit_impl.hh <http://reviews.m5sim.org/r/929/#comment2268> space after ",", here and elsewhere src/cpu/o3/bpred_unit_impl.hh <http://reviews.m5sim.org/r/929/#comment2265> ditto, and below src/cpu/o3/bpred_unit_impl.hh <http://reviews.m5sim.org/r/929/#comment2266> Does this actually exceed 80 columns if it's on one line? src/cpu/pred/tournament.cc <http://reviews.m5sim.org/r/929/#comment2269> Too few spaces in the indent, the {}s aren't really needed (although I'm not too hung up on that), and you could use a ?: if you wanted. src/cpu/pred/tournament.cc <http://reviews.m5sim.org/r/929/#comment2270> The spacing is off here too. Maybe a reviewboard thing? - Gabe On 2011-12-13 10:02:20, Ali Saidi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/929/ > ----------------------------------------------------------- > > (Updated 2011-12-13 10:02:20) > > > Review request for Default. > > > Summary > ------- > > BPred: Fix RAS to handle predicated call/return instructions. > > Change RAS to fix issues with predicated call/return instructions. > Handled all cases in the life of a predicated call and return instruction. > > > Diffs > ----- > > src/cpu/o3/bpred_unit.hh c1ab57ea8805 > src/cpu/o3/bpred_unit_impl.hh c1ab57ea8805 > src/cpu/o3/commit_impl.hh c1ab57ea8805 > src/cpu/pred/tournament.cc c1ab57ea8805 > > Diff: http://reviews.m5sim.org/r/929/diff > > > Testing > ------- > > Works for all regressions tests, however dependent branch predictor patch > exposes x86 issues > > > Thanks, > > Ali > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
