> On Feb. 24, 2015, 5:17 p.m., Steve Reinhardt wrote: > > I like the idea of having a single global limit event, with a function to > > access it so clients can check whether that's what was hit. > > > > However I don't think the GlobalSimLoopExitEvent should be a singleton, as > > there are a bunch of other instances that get created (see exitSimLoop() in > > sim_events.cc). I think it's clearer just to have a global variable for > > limit_event. (One of the extremely rare cases where a global variable is > > OK.) > > > > There's already support in simulate() for first-time initialization with > > the threads_initialized flag. I think we should basically leverage this > > flag to create the global limit_event (rename the flag if you want to make > > it more general). We may want to take the static vars inside simulate() > > and make them all global for consistency; I don't have a strong opinion > > there. > > Curtis Dunham wrote: > Maybe I should just change the comment. I'm not actually making > GlobalSimLoopExitEvent a (simulator-wide) singleton, but rather just a static > with respect to simulate(). Because of how it schedules the event, the > destructor needs to deschedule it. I wrapped simulate()'s instance in a > function to deal with static initialization ordering issues. > > The other uses of GlobalSimLoopExitEvent should still work. > > Steve Reinhardt wrote: > Yea, I read the description first, and my initial reaction on looking at > the code was "if you're going to make it a singleton you should really put it > inside the class", and later I realized "you don't really want a singleton > here", and I didn't really go back to square one to see that it was > not-really-a-singleton *because* you don't really want a singleton. However, > overriding the destructor is a little weird if it only applies to that > instance... can you elaborate a little more on why that's necessary? > > Nevertheless, I still think that just making a plain ol' global variable > is clearer and cleaner. > > Curtis Dunham wrote: > Sure. In summary, it's not strictly necessary: the descheduling upon > destruction is because simulate() doesn't deschedule it, and the destructor > will assert and die if trying to destroy a scheduled event (of any kind - I > personally think this is a bit heavy-handed for an exit event...) Now, we > could change simulate() so that it does deschedule it on every call to > simulate(). I was avoiding the overhead of doing that since it needs to be > scheduled again immediately anyway (more specifically, rescheduled). It just > seemed wasteful to muck around in the event queue that extra time, every 1000 > ticks. Still, I think it could be changed. > > As for the global variable, I had trouble with that due to static/global > object construction in C++. When using gem5 loaded as an .so into another > simulator, it would get to that point and the object wouldn't be constructed. > The standard C++ recipe for that is to wrap it in a method so it gets > constructed on first use. Mildly unpleasant, but not as unpleasant as a > segfault. > > Steve Reinhardt wrote: > Sounds like maybe the right answer is to fix the Event destructor... if > it's checking whether the event is scheduled anyway, maybe it should just > call deschedule() instead of complaining. Maybe print a warning if we're > really worried about it being an issue. > > I was picturing a global pointer variable that you would explicitly > initialize in simulate(), in the one-time-only block that's already there and > protected bu threads_initialized.
Okay, I see what you are suggesting. I will turn the crank on that and repost. As for the Event destructor, while I did find it overly restrictive, I didn't want to get sidetracked in a major refactoring. Were we to issue a warn(), it would be helpful to know what event is being destroyed. But then you realize we've already unwound the polymorphism all the way down to Event and it doesn't have much interesting information... - Curtis ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2665/#review5899 ----------------------------------------------------------- On Feb. 20, 2015, 6:46 p.m., Curtis Dunham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2665/ > ----------------------------------------------------------- > > (Updated Feb. 20, 2015, 6:46 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10706:e77f25583997 > --------------------------- > sim: Reuse the same limit_event in simulate() > > This patch accomplishes two things: > 1. Makes simulate()'s GlobalSimLoopExitEvent a singleton reused > across calls. This is slightly more efficient than recreating > it every time. > 2. Gives callers to simulate() (especially other simulators) a > foolproof way of knowing that the simulation period ended > successfully by hitting the limit event. They can call > getLimitEvent() and compare it to the return > value of simulate(). > > This change was motivated by an ongoing effort to integrate gem5 > and SST, with SST as the master sim and gem5 as the slave sim. > > > Diffs > ----- > > src/sim/sim_events.hh c6cb94a14fea4c59780d73d1623d7031bcede6af > src/sim/simulate.hh c6cb94a14fea4c59780d73d1623d7031bcede6af > src/sim/simulate.cc c6cb94a14fea4c59780d73d1623d7031bcede6af > > Diff: http://reviews.gem5.org/r/2665/diff/ > > > Testing > ------- > > > Thanks, > > Curtis Dunham > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
