----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/910/#review1764 -----------------------------------------------------------
This is converging to where it needs to be. There are style problems thinly scattered throughout so you'll need to fix those, and there are some smaller tweaks I'd like to see. I'm surprised to see indentation issues here and there. You should talk to Ali about setting up vim/emacs to do gem5 style automatically and then you won't have to worry about that. Other more knowledgeable people are already discussing the devices/functional access thing you're doing, so I'll just ignore that. src/arch/arm/isa/insts/misc.isa <http://reviews.m5sim.org/r/910/#comment2274> This line is too long. I won't point out the others that are as well, but please fix throughout. src/arch/arm/tlb.cc <http://reviews.m5sim.org/r/910/#comment2272> What's this for? I don't see any new code in this file that would need this. src/arch/arm/tlb.cc <http://reviews.m5sim.org/r/910/#comment2273> Bad indentation, you don't need the {}s, and you could move it all into the assert. assert(!(timing && functional)); At least fix the indentation, preferably just replace it with this new version. src/cpu/BaseCPU.py <http://reviews.m5sim.org/r/910/#comment2275> Yeah, this assert always seemed a bit hacky to me. src/cpu/CheckerCPU.py <http://reviews.m5sim.org/r/910/#comment2276> What's your reasoning for changing this? Are you worried that this is likely to happen but not signify a real bug? Have you actually seen that happen? src/cpu/DummyChecker.py <http://reviews.m5sim.org/r/910/#comment2277> So much copyright for so little code :-) src/cpu/base.cc <http://reviews.m5sim.org/r/910/#comment2278> It would be better to move these declarations down to where the variables are used. That way you can also merge the #if blocks. src/cpu/base.cc <http://reviews.m5sim.org/r/910/#comment2279> Have you tested how the checker acts when you switch CPUs? It wouldn't be criminal if that isn't working perfectly at this stage since this change already fixing alot, but it would be nice if that worked. src/cpu/base_dyn_inst.hh <http://reviews.m5sim.org/r/910/#comment2280> Bad indentation. src/cpu/checker/cpu.cc <http://reviews.m5sim.org/r/910/#comment2281> A lot of this could be moved to the initializer list. It's ok to leave it here, though, since this isn't performance sensitive. src/cpu/checker/cpu.cc <http://reviews.m5sim.org/r/910/#comment2282> Drop the {}s. src/cpu/checker/cpu.cc <http://reviews.m5sim.org/r/910/#comment2283> I think you're only indented 3 spaces here instead of 4. src/cpu/checker/cpu.cc <http://reviews.m5sim.org/r/910/#comment2284> It's really only going to rot, and it'll be in version control anyway. I'd leave a comment saying you deleted it and just get rid of it. src/cpu/checker/cpu.cc <http://reviews.m5sim.org/r/910/#comment2285> Bracket should be on the preceding line. src/cpu/checker/cpu.cc <http://reviews.m5sim.org/r/910/#comment2286> No {}s, indentation. src/cpu/checker/cpu.cc <http://reviews.m5sim.org/r/910/#comment2288> spaces after , src/cpu/checker/cpu.cc <http://reviews.m5sim.org/r/910/#comment2287> Using ?: here makes this if harder to understand. You should declare a temporary right above this if, maybe extraDataOk. Using the ?: there would be ok since that would be simpler to start with. src/cpu/checker/cpu_impl.hh <http://reviews.m5sim.org/r/910/#comment2290> No offset involved... src/cpu/checker/cpu_impl.hh <http://reviews.m5sim.org/r/910/#comment2289> It would be better to call this handlePendingInt since it's a more ISA neutral term and is more consistent with the rest of gem5. src/cpu/checker/cpu_impl.hh <http://reviews.m5sim.org/r/910/#comment2291> Drop the {}s. src/cpu/checker/cpu_impl.hh <http://reviews.m5sim.org/r/910/#comment2292> You should declare a temporary which has the inst to check and then have one if and one panic which checks it. That gets rid of the redundancy here. src/cpu/checker/cpu_impl.hh <http://reviews.m5sim.org/r/910/#comment2293> This line is gigantic. Wrap it, and/or use a temporary. src/cpu/checker/cpu_impl.hh <http://reviews.m5sim.org/r/910/#comment2294> Drop the {}s, result.pop(); on the next line. src/cpu/checker/cpu_impl.hh <http://reviews.m5sim.org/r/910/#comment2295> ckFault => checkFault. It's not very long and there's no reason to be cryptic. src/cpu/checker/cpu_impl.hh <http://reviews.m5sim.org/r/910/#comment2296> Not here you aren't... src/cpu/checker/cpu_impl.hh <http://reviews.m5sim.org/r/910/#comment2297> Delete instead of commenting it out. I know the code you copied it from had this commented out and eventually I want to fix this, but there's no reason to propagate the problem. src/cpu/checker/cpu_impl.hh <http://reviews.m5sim.org/r/910/#comment2298> If the checker CPU is going to be able to trace things, it should have a tracer parameter. If not, pass NULL in as the tracer argument to execute. I'm 95% sure that will work, and if not you should be able to tell pretty quickly. src/cpu/checker/cpu_impl.hh <http://reviews.m5sim.org/r/910/#comment2299> I'm not sure it makes sense for the checker CPU to recognize PC based events since the main CPU is going to do that. It may, though. src/cpu/checker/cpu_impl.hh <http://reviews.m5sim.org/r/910/#comment2301> Bad indentation. src/cpu/checker/cpu_impl.hh <http://reviews.m5sim.org/r/910/#comment2302> Could you clarify what you mean here? The PC mapping to an integer register should be handled by the instructions and the decoder, although I can believe there may be subtle side effects. Also, vagarities => vagaries. src/cpu/o3/O3Checker.py <http://reviews.m5sim.org/r/910/#comment2303> I remember seeing this line before so I may have already asked, but why are you changing this? src/cpu/o3/commit_impl.hh <http://reviews.m5sim.org/r/910/#comment2304> Drop the {}s, and I already mentioned this function's name earlier. src/cpu/o3/dyn_inst_impl.hh <http://reviews.m5sim.org/r/910/#comment2305> {}s src/cpu/o3/fetch_impl.hh <http://reviews.m5sim.org/r/910/#comment2306> What are these for? If you added these for a reason please say so, but otherwise be careful not to add dead includes. src/cpu/o3/thread_context_impl.hh <http://reviews.m5sim.org/r/910/#comment2307> Drop the {}s. I'll stop pointing these out (and I may have missed some), but please fix throughout. - Gabe On 2011-12-13 13:25:24, Geoffrey Blake wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/910/ > ----------------------------------------------------------- > > (Updated 2011-12-13 13:25:24) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > CheckerCPU: Re-factor CheckerCPU to be compatible with current gem5 > > Brings the CheckerCPU back to a functioning state allowing FS and SE mode > checking of the O3CPU. These changes have only been tested with the > ARM ISA. Other ISAs will potentially require modification. > > > Diffs > ----- > > configs/common/FSConfig.py c1ab57ea8805 > src/arch/arm/isa.cc c1ab57ea8805 > src/arch/arm/isa/insts/m5ops.isa c1ab57ea8805 > src/arch/arm/isa/insts/misc.isa c1ab57ea8805 > src/arch/arm/table_walker.hh c1ab57ea8805 > src/arch/arm/table_walker.cc c1ab57ea8805 > src/arch/arm/tlb.hh c1ab57ea8805 > src/arch/arm/tlb.cc c1ab57ea8805 > src/arch/arm/utility.cc c1ab57ea8805 > src/cpu/BaseCPU.py c1ab57ea8805 > src/cpu/CheckerCPU.py c1ab57ea8805 > src/cpu/DummyChecker.py PRE-CREATION > src/cpu/SConscript c1ab57ea8805 > src/cpu/base.cc c1ab57ea8805 > src/cpu/base_dyn_inst.hh c1ab57ea8805 > src/cpu/base_dyn_inst_impl.hh c1ab57ea8805 > src/cpu/checker/cpu.hh c1ab57ea8805 > src/cpu/checker/cpu.cc c1ab57ea8805 > src/cpu/checker/cpu_impl.hh c1ab57ea8805 > src/cpu/checker/thread_context.hh c1ab57ea8805 > src/cpu/dummy_checker_builder.cc PRE-CREATION > src/cpu/o3/O3CPU.py c1ab57ea8805 > src/cpu/o3/O3Checker.py c1ab57ea8805 > src/cpu/o3/checker_builder.cc c1ab57ea8805 > src/cpu/o3/commit_impl.hh c1ab57ea8805 > src/cpu/o3/cpu.hh c1ab57ea8805 > src/cpu/o3/cpu.cc c1ab57ea8805 > src/cpu/o3/dyn_inst_impl.hh c1ab57ea8805 > src/cpu/o3/fetch_impl.hh c1ab57ea8805 > src/cpu/o3/iew_impl.hh c1ab57ea8805 > src/cpu/o3/lsq_unit_impl.hh c1ab57ea8805 > src/cpu/o3/thread_context.hh c1ab57ea8805 > src/cpu/o3/thread_context_impl.hh c1ab57ea8805 > src/cpu/simple/BaseSimpleCPU.py c1ab57ea8805 > src/cpu/simple/base.hh c1ab57ea8805 > src/cpu/simple/base.cc c1ab57ea8805 > src/cpu/simple_thread.hh c1ab57ea8805 > src/cpu/thread_context.hh c1ab57ea8805 > src/dev/Device.py c1ab57ea8805 > src/dev/isa_fake.hh c1ab57ea8805 > src/dev/isa_fake.cc c1ab57ea8805 > > Diff: http://reviews.m5sim.org/r/910/diff > > > Testing > ------- > > Successfully runs gzip in SE mode. Successfully boots linux kernel in FS > mode. Works with checkpoints and fast-forwarding. Testing with buggy O3CPUs > to test checker's capabilities. > > > Thanks, > > Geoffrey > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
