I'll address these review comments as soon as possible. In testing the Checker on actual bugs found in the O3 for ARM I've found the Checker is not catching them. The problem is the Checker is too tightly coupled to the O3 model and will follow the O3 off the rails. I'm in the process of trying to fix this.
Geoff On Sun, Nov 6, 2011 at 6:38 PM, nathan binkert <n...@binkert.org> wrote: > Given that the checker CPU doesn't work at all right now and hasn't for > quite some time, I have no problem with letting all sorts of things slide > if they get us closer to a working system. > > Nate > > On Sun, Nov 6, 2011 at 1:51 AM, Gabe Black <gbl...@eecs.umich.edu> wrote: > >> This is an automatically generated e-mail. To reply, visit: >> http://reviews.m5sim.org/r/907/ >> >> I'm glad to see the Checker CPU brought back from the dead, and I'm >> especially glad I didn't have to do it myself :-). This is a pretty big >> patch, and there are a number of places where things need to be fixed, there >> were questions, clarifications, etc. This isn't ready to commit but it's a >> solid start. >> >> I would request this gets broken into smaller patches that do one thing at a >> time instead of doing all this at once, but at this point that's probably >> impractical. Please shoot for that in the future though. >> >> >> >> src/cpu/BaseCPU.py<http://reviews.m5sim.org/r/907/diff/1/?file=15437#file15437line178> >> (Diff >> revision 1) >> >> def connectAllPorts(self, cached_bus, uncached_bus = None): >> >> 178 >> >> print "In add private split L1 caches" >> >> Get rid of debugging output. >> >> >> >> src/cpu/BaseCPU.py<http://reviews.m5sim.org/r/907/diff/1/?file=15437#file15437line196> >> (Diff >> revision 1) >> >> def connectAllPorts(self, cached_bus, uncached_bus = None): >> >> 196 >> >> # "checker.itb.walker.port", >> "checker.dtb.walker.port"] >> >> What's this doing here? >> >> >> >> src/cpu/base_dyn_inst.hh<http://reviews.m5sim.org/r/907/diff/1/?file=15440#file15440line568> >> (Diff >> revision 1) >> >> class BaseDynInst : public FastAlloc, public RefCounted >> >> 568 >> >> uint64_t popIntResult() >> >> This whole result thing is a relic that needs to go away. You shouldn't >> build your functionality on top of it. The results of the instruction are >> the memory request it generates, if any, and the destination registers it >> sets. You should frame things in those terms. >> >> I see you're changing the instResult from a scalar value to a queue which >> partially corrects one of its deficiencies, but it's still a redundant and >> obsolete mechanism. >> >> >> >> src/cpu/checker/cpu.cc<http://reviews.m5sim.org/r/907/diff/1/?file=15443#file15443line258> >> (Diff >> revision 1) >> >> CheckerCPU::unserialize(Checkpoint *cp, const string §ion) >> >> 230 >> >> #if 0 >> >> Don't comment out code, delete it. >> >> >> >> src/cpu/checker/cpu_impl.hh<http://reviews.m5sim.org/r/907/diff/1/?file=15444#file15444line144> >> (Diff >> revision 1) >> >> Checker<DynInstPtr>::verify(DynInstPtr &completed_inst) >> >> void >> >> 101 >> >> while (1) { >> >> 143 >> >> while (1) { >> >> This fetch logic is as gnarly as it is because O3 is doing branch >> prediction, microcode extraction, fetching a cache line at a time, blah blah >> blah. If you're doing simpler functional execution modeling you might get >> away with something a lot simpler modeled off of, say, the simple CPU. I'm >> not sure how the Checker CPU works in this area (or most others). >> >> >> >> src/cpu/checker/cpu_impl.hh<http://reviews.m5sim.org/r/907/diff/1/?file=15444#file15444line156> >> (Diff >> revision 1) >> >> Checker<DynInstPtr>::verify(DynInstPtr &completed_inst) >> >> void >> >> 111 >> >> // maintain $r0 semantics >> >> 155 >> >> //maintain $r0 semantics XXX For ARM this isn't true? >> >> Yes it is. It's unnecessary but accommodated. One of these days I'm >> planning to handle this in a better, less Alpha-y way. >> >> >> >> src/cpu/checker/cpu_impl.hh<http://reviews.m5sim.org/r/907/diff/1/?file=15444#file15444line158> >> (Diff >> revision 1) >> >> Checker<DynInstPtr>::verify(DynInstPtr &completed_inst) >> >> void >> >> 113 >> >> #ifdef TARGET_ALPHA >> >> 157 >> >> #ifdef TARGET_ALPHA >> >> This is another Alpha-ism I'd like to clean up one of these days. >> >> >> >> src/cpu/checker/cpu_impl.hh<http://reviews.m5sim.org/r/907/diff/1/?file=15444#file15444line465> >> (Diff >> revision 1) >> >> Checker<DynInstPtr>::verify(DynInstPtr &completed_inst) >> >> void >> >> 441 >> >> } // Checker CPU isn't completely exact in matching desitination >> registers >> >> The instructions themselves sort out if writes to registers are really >> using registers or the PC. From the CPU's perspective this should all be >> abstracted away. >> >> >> >> src/cpu/checker/thread_context.hh<http://reviews.m5sim.org/r/907/diff/1/?file=15445#file15445line272> >> (Diff >> revision 1) >> >> class CheckerThreadContext : public ThreadContext >> >> 266 >> >> DPRINTF(Checker, "Setting misc reg with no effect: %d to both >> Checker and O3..\n", misc_reg); >> >> Line length? >> >> >> >> src/cpu/checker/thread_context.hh<http://reviews.m5sim.org/r/907/diff/1/?file=15445#file15445line279> >> (Diff >> revision 1) >> >> class CheckerThreadContext : public ThreadContext >> >> 273 >> >> DPRINTF(Checker, "Setting misc reg with effect: %d to both Checker >> and O3..\n", misc_reg); >> >> Line length? >> >> >> >> src/cpu/o3/fetch_impl.hh<http://reviews.m5sim.org/r/907/diff/1/?file=15453#file15453line46> >> (Diff >> revision 1) >> >> 46 >> >> #include <list> >> >> What are these for? I don't see anything changing here that would suggest >> you need them now but didn't before, although that's not to say that you >> needed them before and didn't have them. If these are leftovers please get >> rid of them. >> >> >> >> src/cpu/simple_thread.hh<http://reviews.m5sim.org/r/907/diff/1/?file=15458#file15458line310> >> (Diff >> revision 1) >> >> class SimpleThread : public ThreadState >> >> return NULL; } >> >> 310 >> >> // XXX: Fixes a array out of bounds compiler error when checkercpu >> enabled >> >> This seems like a more serious problem that should actually be tracked >> down rather than worked around. Maybe NumFloatRegs isn't high enough? Maybe >> the index isn't scaled/offset/whatever properly? >> >> >> >> src/cpu/thread_context.hh<http://reviews.m5sim.org/r/907/diff/1/?file=15459#file15459line210> >> (Diff >> revision 1) >> >> class ThreadContext >> >> 210 >> >> virtual void pcStateNoRecord(const TheISA::PCState &val) = 0; >> >> Does this belong in the ThreadContext class? I doubt it, but I'm not >> completely sure. >> >> >> >> src/mem/packet.hh<http://reviews.m5sim.org/r/907/diff/1/?file=15461#file15461line280> >> (Diff >> revision 1) >> >> class Packet : public FastAlloc, public Printable >> >> 280 >> >> * Would use a bitset if it was able to by dynamically allocated. >> >> by => be >> >> >> >> src/mem/packet.cc<http://reviews.m5sim.org/r/907/diff/1/?file=15462#file15462line161> >> (Diff >> revision 1) >> >> bool >> >> 161 >> >> Packet::checkFunctionalComplete() >> >> What's this function used for? There's a pretty decent chance I just >> missed it, but I didn't see where it was called by anything. If it's not >> used, then we don't need to make any changes to the Packet class. >> >> >> - Gabe >> >> On November 3rd, 2011, 1:32 p.m., Ali Saidi wrote: >> Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and >> Nathan Binkert. >> By Ali Saidi. >> >> *Updated 2011-11-03 13:32:25* >> Description >> >> CheckerCPU: Re-factor CheckerCPU to be compatible current state of gem5 >> >> Brings the CheckerCPU back to a functioning state allowing FS and SE mode >> checking of the O3CPU. Lacking compatibility with checkpoints and >> fast-forwarding currently. These changes have only been tested on the >> ARM ISA. Other ISAs should work, but some modification will be required. >> >> Diffs >> >> - build_opts/ARM_wCHECKER_FS (PRE-CREATION) >> - build_opts/ARM_wCHECKER_SE (PRE-CREATION) >> - src/arch/arm/isa.cc (5fb918115c07) >> - src/arch/arm/isa/insts/m5ops.isa (5fb918115c07) >> - src/arch/arm/isa/insts/misc.isa (5fb918115c07) >> - src/arch/arm/table_walker.hh (5fb918115c07) >> - src/arch/arm/table_walker.cc (5fb918115c07) >> - src/arch/arm/tlb.hh (5fb918115c07) >> - src/arch/arm/tlb.cc (5fb918115c07) >> - src/arch/arm/utility.cc (5fb918115c07) >> - src/cpu/BaseCPU.py (5fb918115c07) >> - src/cpu/CheckerCPU.py (5fb918115c07) >> - src/cpu/base.cc (5fb918115c07) >> - src/cpu/base_dyn_inst.hh (5fb918115c07) >> - src/cpu/base_dyn_inst_impl.hh (5fb918115c07) >> - src/cpu/checker/cpu.hh (5fb918115c07) >> - src/cpu/checker/cpu.cc (5fb918115c07) >> - src/cpu/checker/cpu_impl.hh (5fb918115c07) >> - src/cpu/checker/thread_context.hh (5fb918115c07) >> - src/cpu/o3/O3CPU.py (5fb918115c07) >> - src/cpu/o3/O3Checker.py (5fb918115c07) >> - src/cpu/o3/checker_builder.cc (5fb918115c07) >> - src/cpu/o3/commit_impl.hh (5fb918115c07) >> - src/cpu/o3/cpu.hh (5fb918115c07) >> - src/cpu/o3/cpu.cc (5fb918115c07) >> - src/cpu/o3/dyn_inst_impl.hh (5fb918115c07) >> - src/cpu/o3/fetch_impl.hh (5fb918115c07) >> - src/cpu/o3/iew_impl.hh (5fb918115c07) >> - src/cpu/o3/lsq_unit_impl.hh (5fb918115c07) >> - src/cpu/o3/thread_context.hh (5fb918115c07) >> - src/cpu/o3/thread_context_impl.hh (5fb918115c07) >> - src/cpu/simple_thread.hh (5fb918115c07) >> - src/cpu/thread_context.hh (5fb918115c07) >> - src/mem/bus.cc (5fb918115c07) >> - src/mem/packet.hh (5fb918115c07) >> - src/mem/packet.cc (5fb918115c07) >> >> View Diff <http://reviews.m5sim.org/r/907/diff/> >> > _______________________________________________ > gem5-dev mailing list > gem5-dev@gem5.org > http://m5sim.org/mailman/listinfo/gem5-dev > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
