----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/748/#review1337 -----------------------------------------------------------
src/arch/mips/isa/formats/branch.isa <http://reviews.m5sim.org/r/748/#comment1790> Great, thanks for the quick code review and change. src/cpu/inorder/inorder_dyn_inst.cc <http://reviews.m5sim.org/r/748/#comment1785> In general, commenting out code in patches is not a good practice as it leads to lots of old text floating through code. If the old code has some remaining purpose, then we should place a comment there for future reference. Otherwise, the best thing to do is just delete it. Can you update this to either delete that comment or place a note why that code is still there? Assuming this patch has been tested for the MIPS regressions we have (and you have), this looks good. Lastly, I want to test on a SPARC hello world regression that I have. src/cpu/inorder/resources/branch_predictor.cc <http://reviews.m5sim.org/r/748/#comment1786> I'd like to avoid "#if ISA_HAS_DELAY_SLOT" as much as possible in code that's not in src/arch/*, but this may be a spot where it's necessary src/cpu/inorder/resources/fetch_seq_unit.cc <http://reviews.m5sim.org/r/748/#comment1787> same as above. src/cpu/inorder/resources/fetch_seq_unit.cc <http://reviews.m5sim.org/r/748/#comment1788> This will work for all architectures right without the "#if"? No other defined ISAs mark that CondDelaySlot() flag. I dont think saving the check here will be worth the clutter. Can you remove then test some alpha regressions? Maybe just the hello world-quick-alpha regression. and the 70.twolf-long-alpha regression? src/cpu/inorder/resources/fetch_seq_unit.cc <http://reviews.m5sim.org/r/748/#comment1789> same as above. Checking for that cond-delay-slot flag already separates out the MIPS code. We can get rid of the #if and possibly put a comment below (or beside that line w/in 80 characters) saying that the 2nd condition handles MIPS branch likely instructions. - Korey On 2011-06-20 03:26:16, Deyuan Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/748/ > ----------------------------------------------------------- > > (Updated 2011-06-20 03:26:16) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > Make the newly gem5 support mips branch likely instruction again. > Fix 4 files: > src/arch/mips/isa/formats/branch.isa > src/cpu/inorder/inorder_dyn_inst.cc > src/cpu/inorder/resources/branch_predictor.cc > src/cpu/inorder/resources/fetch_seq_unit.cc > > > Diffs > ----- > > src/arch/mips/isa/formats/branch.isa d9f54de93703 > src/cpu/inorder/inorder_dyn_inst.cc d9f54de93703 > src/cpu/inorder/resources/branch_predictor.cc d9f54de93703 > src/cpu/inorder/resources/fetch_seq_unit.cc d9f54de93703 > > Diff: http://reviews.m5sim.org/r/748/diff > > > Testing > ------- > > Tested. > > > Thanks, > > Deyuan > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
