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 <[email protected]> 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 [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
