> On June 21, 2013, 8:44 p.m., Steve Reinhardt wrote: > > This looks very good to me... I'm pleasantly surprised by how modest the > > changes are. In addition to addressing the issues that Andrew brought up, > > I suggest: > > > > 1. Adding a few high-level comments describing what's being done. I'm not > > a SystemC user, but I can still make some guesses about what's happening; > > nevertheless, a few brief English sentences in comments here and there > > would go a long way toward making me confident that my guesses are correct. > > > > 2. Wrapping the async event stuff in a common API, so that instead of (for > > example) having > > #ifdef _SYSTEMC_ > > async_event.notify(); > > #else > > async_event = true; > > #endif > > repeated all over the place, we can have a single set of functions that get > > called unconditionally, with a minimum number of unique #ifdefs hidden > > behind that interface. > > > > 3. Finding a way other than the 'sysc_build' name to trigger the SystemC > > compilation. We already have a mechanism for compile-time configuration > > flags (the "sticky" vars, like USE_FENV and TARGET_ISA). Is there a good > > reason not to just add a 'USE_SYSTEMC' flag there and skip all the > > 'sysc_build' changes?
Let me second that praise of how simple the patch is and how well it 99% "just worked" for me. - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1928/#review4458 ----------------------------------------------------------- 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
