> On July 19, 2012, 12:52 p.m., Nathan Binkert wrote: > > src/sim/clocked_object.hh, line 95 > > <http://reviews.gem5.org/r/1303/diff/3/?file=28042#file28042line95> > > > > This is likely premature optimization, but sticking a div in every > > clock schedule seems scary. The trick that I've used in the past was to > > save the clock tick and check to see if curtick is == to the saved tick. > > If so, you can just do an add and almost all the time, the condition will > > pass. Again, perhaps over optimized. I'll send you the code. > > Andreas Hansson wrote: > I guess it all boils down to how it is used. Ultimately gem5 should be > event based, not a collection of cycle callable models. As such, I would hope > it doesn't get called every cycle. I have not stats to back that up though, > so it could be that you're absolutely right. > > At this point, I merely wanted to tidy up what was there...that did a > modulo operation for every call. > > Nathan Binkert wrote: > Sure it's event based, but most things get clocked every cycle for many > cycles and then go idle for a period of time and then do it again. (Think > CPUs, caches). In this case, the addition works except from waking after > idle. When that happens, the condition fails, and then you do the divide. > > Andreas Hansson wrote: > I can give it a bash and compare the performance. Perhaps as a follow on > patch or you want to see it as part of this one? > > Nathan Binkert wrote: > As a follow-on is fine with me. I just get concerned with event queue > performance because in certain situations the performance can be particularly > bad. As I recall, AMD had some use of a really old version of m5 (that ali > did for them) that really stressed the event queue and ran into similar > problems. If we ever get the network model in ruby moved over to our event > queue, and people run network only experiments, you'll certainly see > performance issues. > > Andreas Hansson wrote: > Are you concerned how this performs compared to what it used to be (see > below)? If so I can change it back. If the issue is more forward looking I'd > prefer to keep it for a future patch. > > Tick > BaseCPU::nextCycle(Tick begin_tick) > { > Tick next_tick = begin_tick; > if (next_tick % clock != 0) > next_tick = next_tick - (next_tick % clock) + clock; > next_tick += phase; > assert(next_tick >= curTick()); > return next_tick; > }
Oh, I didn't realize that the current version had a modulo. Yeah, you can do it in a future patch. I'm mostly concerned that this starts getting used for objects/events that are much shorter lived such that the event queue time starts to dominate. I thought that there was an example of what I was talking about in the tree, but after looking, it must have been in my HP stuff or got removed. - Nathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1303/#review3101 ----------------------------------------------------------- On July 19, 2012, 5:46 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1303/ > ----------------------------------------------------------- > > (Updated July 19, 2012, 5:46 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9127:f00508d2c790 > --------------------------- > Clock: Move the clock and related functions to ClockedObject > > This patch moves the clock of the CPU, bus, and numerous devices to > the new class ClockedObject, that sits in between the SimObject and > MemObject in the class hierarchy. Although there are currently a fair > amount of MemObjects that do not make use of the clock, they > potentially should do so, e.g. the caches should at some point have > the same clock as the CPU, potentially with a 1:n ratio. This patch > does not introduce any new clock objects or object hierarchies > (clusters, clock domains etc), but is still a step in the direction of > having a more structured approach clock domains. > > The most contentious part of this patch is the serialisation of clocks > that some of the modules (but not all) did previously. This > serialisation should not be needed as the clock is set through the > parameters even when restoring from the checkpoint. In other words, > the state is "stored" in the Python code that creates the modules. > > The nextCycle methods are also simplified and the clock phase > parameter of the CPU is removed (this could be part of a clock object > once they are introduced). > > > Diffs > ----- > > src/arch/x86/interrupts.hh 48eeef8a0997 > src/arch/x86/interrupts.cc 48eeef8a0997 > src/arch/x86/utility.cc 48eeef8a0997 > src/cpu/BaseCPU.py 48eeef8a0997 > src/cpu/base.hh 48eeef8a0997 > src/cpu/base.cc 48eeef8a0997 > src/cpu/testers/memtest/memtest.hh 48eeef8a0997 > src/cpu/testers/networktest/networktest.hh 48eeef8a0997 > src/dev/CopyEngine.py 48eeef8a0997 > src/dev/Ethernet.py 48eeef8a0997 > src/dev/arm/RealView.py 48eeef8a0997 > src/dev/arm/pl111.hh 48eeef8a0997 > src/dev/arm/pl111.cc 48eeef8a0997 > src/dev/arm/timer_cpulocal.cc 48eeef8a0997 > src/dev/i8254xGBe.hh 48eeef8a0997 > src/dev/i8254xGBe.cc 48eeef8a0997 > src/dev/ns_gige.hh 48eeef8a0997 > src/dev/ns_gige.cc 48eeef8a0997 > src/dev/sinic.hh 48eeef8a0997 > src/dev/sinic.cc 48eeef8a0997 > src/mem/Bus.py 48eeef8a0997 > src/mem/MemObject.py 48eeef8a0997 > src/mem/bus.hh 48eeef8a0997 > src/mem/bus.cc 48eeef8a0997 > src/mem/mem_object.hh 48eeef8a0997 > src/mem/mem_object.cc 48eeef8a0997 > src/sim/ClockedObject.py PRE-CREATION > src/sim/SConscript 48eeef8a0997 > src/sim/clocked_object.hh PRE-CREATION > > Diff: http://reviews.gem5.org/r/1303/diff/ > > > Testing > ------- > > util/regress all passing (disregarding t1000 and eio) > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
