> On June 20, 2013, 8:50 a.m., Andrew Bardsley wrote: > > src/sim/eventq.hh, line 54 > > <http://reviews.gem5.org/r/1928/diff/1/?file=36233#file36233line54> > > > > (I'm attaching this comment here as the eventq.cc patch isn't currently > > visible) > > > > I've tried a small example passing gem5 memory request around using the > > SystemC patch. I've hit a problem with issuing gem5 events from SystemC as > > getCurTick() is allowed to lag sc_time_stamp. > > > > Both EventQueue::insert and EventQueue::remove use setCurTick to > > correct the time difference but it's quite a common paradigm in gem5 to do: > > > > event.schedule(... getCurTick() + something ...) > > > > Where getCurTick will return the wrong value. > > > > Obviously simple cases can be solved by using the 'right' source of > > time (and I can imagine scenarios where you might actually want to > > cultivate a time difference if you were, say, to have some kind of quantum > > keeping between SystemC and gem5). Unfortunately, when the getCurTick is > > deep in code you have no control of (my immediate problem was with a > > schedule in SimpleMemory called from sendTimingReq from SystemC) it's > > harder to solve. > > > > I'd suggest a few solutions: > > > > 1) Make the two times *always* agree. Use sc_time_stamp().value() for > > getCurTick. > > > > 2) Provide a 'catchUp' member function that can be called before any > > potentially curTick-accessing event. > > > > In both cases I'd suggest that the current setCurTicks in the SystemC > > modified EventQueue use an internal 'catchUp' and that that function should > > assert that the gem5 time is earlier than the SystemC time and that no > > events are currently scheduled between the two times. > > > > There is another problem. EventQueue isn't actually used only for > > mainEventQueue. > > > > The CPU models use (very ugly in my opinion) EventQueues to sequence > > events based on instruction counts rather than Tick time. > > > > Look at line 140 in src/cpu/base.cc for a declaration and the function > > instDone at line 1523 in src/cpu/o3/cpu.cc for examples. > > > > I would suggest, to solve both of these problems, that when using the > > SystemC extension EventQueue should have a shallow class hierarchy. > > > > Have EventQueue in its current, non SystemC form as the base (with the > > 'base is most common implementation' philosophy), make either the > > insert/remove/getCurTick interfaces virtual (or conditionally virtual if > > the virtual call scares people) or have a virtual 'synchronise/catchUp' > > that is empty in EventQueue and override those functions in a > > SystemCEventQueue derived class of which mainEventQueue is the only (rare) > > instance. > > > > Having that derived class would also give a home to the Gem5EventQueue > > class/instance and EventQueue::returnEvent. > > > > The event queue notify in Event::insertBefore could be implemented by > > an empty EventQueue::notifyInserted function overridden in > > SystemCEventQueue. > > Steve Reinhardt wrote: > If it's helpful to create a separate EventQueue-like class to handle the > instruction-count-based events, I don't have an objection to that.
Putting some separation between the Tick-ordered EventQueue and one based on counts *would* be nice. If that 'CounterEventQueue' and the time-based EventQueue didn't share a common base, you might need two Event classes (to accommodate references to EventQueue in Event) which seems a bit unfortunate. Does anyone have a good feel (from maybe the profiling that gave rise to the Event::nextBin/nextInBin optimisation) for the cost of adding the virtual features like those described in my first comment to the EventQueue/Event actions? I've not done the experiment but there's significant objection to messing with the structure of EventQueue then I withdraw the above suggestions. Also, what's the feeling on feature-conditional 'virtual' qualifiers on functions? #ifdef _SYSTEMC_ #define SYSTEMC_VIRTUAL virtual #else #define SYSTEMC_VIRTUAL #endif Is it considered as undesirable as it is ugly? - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1928/#review4452 ----------------------------------------------------------- On June 18, 2013, 8:43 a.m., Frédéric Konrad wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1928/ > ----------------------------------------------------------- > > (Updated June 18, 2013, 8:43 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > This allows Gem5 to be build with SystemC kernel when build with 'scons > sysc_build/*'. > > NOTE: There is a small bug with SystemC 2.3.0 which is the following: > > systemc-2.3.0/include/sysc/communication/sc_interface.h:72: error: > "__SUNPRO_CC" is not defined > > and can be fixed by replacing > > private: > static sc_event m_never_notified; > #if __SUNPRO_CC == 0x520 > // Workaround for a bug in the Sun WorkShop 6 update 2 compiler. > // An empty virtual base class can cause the optimizer to > // generate wrong code. > char dummy; > #endif > }; > > by > > private: > static sc_event m_never_notified; > #if defined(__SUNPRO_CC) && (__SUNPRO_CC == 0x520) > // Workaround for a bug in the Sun WorkShop 6 update 2 compiler. > // An empty virtual base class can cause the optimizer to > // generate wrong code. > char dummy; > #endif > }; > > > Diffs > ----- > > SConstruct UNKNOWN > src/base/pollevent.cc UNKNOWN > src/python/swig/event.i UNKNOWN > src/python/swig/pyevent.cc UNKNOWN > src/sim/async.hh UNKNOWN > src/sim/async.cc UNKNOWN > src/sim/eventq.hh UNKNOWN > src/sim/eventq.cc UNKNOWN > src/sim/init.cc UNKNOWN > src/sim/main.cc UNKNOWN > src/sim/simulate.hh UNKNOWN > src/sim/simulate.cc UNKNOWN > > Diff: http://reviews.gem5.org/r/1928/diff/ > > > Testing > ------- > > > Thanks, > > Frédéric Konrad > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
