> On 2010-07-10 08:52:15, Steve Reinhardt wrote:
> > src/sim/eventq.hh, line 489
> > <http://reviews.m5sim.org/r/51/diff/1/?file=755#file755line489>
> >
> >     I don't like getting rid of this assertion... it's actually pretty 
> > useful in knowing when something's not right.  You should add some code 
> > upstream somewhere to skip adding the event if we're already past it.
> 
> Timothy Jones wrote:
>     So the main problem with this assertion is that the event I'm trying to 
> add is triggered by an instruction count, not a certain Tick.  I'm happy to 
> either add code somewhere else to avoid adding old events or to put this 
> assertion back in and find a new way of adding instruction-count-based 
> events.  Which would be the preferred option?
> 
> Steve Reinhardt wrote:
>     I understand that... but even with instructions, I think you'd have the 
> same problem, which is that you're scheduling an event that's never going to 
> get triggered.  So if you want a callback after 1000 insts are executed, but 
> 2000 insts have already been executed, it seems like you'd still need to 
> check somewhere before you call schedule() and realize that there's no point 
> in calling schedule() because this event is not going to happen.  I don't 
> think instruction-based events should be treated differently.
> 
> Ali Saidi wrote:
>     I ran into this problem a few weeks ago and have just removed the 
> assertion in my tree. It's not ideal, but i had no idea how to solve it in 
> any generic fashion. The best I could come up with was adding a parameter to 
> the event queue constructor that specified it wasn't tick based, although 
> getting to current count to compare it to is still problematic.
>
> 
> Steve Reinhardt wrote:
>     I see... the problem is that it's comparing with curTick and not the 
> actual thing that's being used to drive the queue processing.  Duh, I missed 
> that before.  We'll run into a similar problem when we parallelize the 
> simulation and have per-thread definitions of curTick.  Seems like we need to 
> have the EventQueue class have a pointer to the curTick-equivalent variable 
> for that queue... is it reasonably straightforward to provide that at the 
> point where each queue is created?  Is there any scenario in which that 
> wouldn't work, such as a queue would switch variables midstream?  I don't 
> think so.
> 
> Nathan Binkert wrote:
>     I actually believe that curTick shouldn't be a global variable.  The 
> class itself should have an internal curTick variable that can be accessed.  
> (Probably better if it were a curTick() function).  That said, if we leave it 
> as a variable, we could make curTick a Tick & to mainEventQueue.current.  As 
> Steve alluded to though, we're going to run into issues will parallelization.

So I think the only question is whether EventQueue should contain the tick 
variable or just have a pointer/reference to it... for things like 
instruction-count-based queues in particular it makes more sense for the 
counter to live outside and have the queue just reference it.


> On 2010-07-10 08:52:15, Steve Reinhardt wrote:
> > src/sim/sim_object.cc, line 275
> > <http://reviews.m5sim.org/r/51/diff/1/?file=757#file757line275>
> >
> >     I'd prefer a more informative message like "Error: setMaxInsts called 
> > on non-CPU" (and same with the following function).
> >     
> >     There's a broader question of whether this is how we want to expose 
> > subclass-specific methods to Python... I don't have any great ideas for 
> > alternatives, but Nate might.
> 
> Timothy Jones wrote:
>     I can definitely alter the error message here and can make alterations to 
> the way this is exposed to python, if necessary.
> 
> Steve Reinhardt wrote:
>     Thanks... changing the error message is all I need to be happy.  I'd like 
> to hear Nate's opinion on whether it's good to end up with a bunch of 
> class-specific methods on SimObject, and what the alternatives might be, but 
> until he speaks up you can leave the rest of it like it is.
> 
> Nathan Binkert wrote:
>     Actually, I don't think these should be done that way.  System.py, and 
> src/python/swig/system.i Illustrate another way.  It's somewhat messy if we 
> want to do that a lot though.  We could try to generalize it.

We still have this in SimObject.py:

    def getMemoryMode(self):
        if not isinstance(self, m5.objects.System):
            return None

        return self._ccObject.getMemoryMode()

    def changeTiming(self, mode):
        if isinstance(self, m5.objects.System):
            # i don't know if there's a better way to do this - calling
            # setMemoryMode directly from self._ccObject results in calling
            # SimObject::setMemoryMode, not the System::setMemoryMode
            self._ccObject.setMemoryMode(mode)

I don't see why those methods are necessary; assuming they're not, and that we 
get rid of them, and that there are only a limited number of classes that 
define their own python methods, then I think this approach is not too bad.  
How would you generalize it?


- Steve


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/51/#review65
-----------------------------------------------------------


On 2010-07-09 18:14:44, Timothy Jones wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.m5sim.org/r/51/
> -----------------------------------------------------------
> 
> (Updated 2010-07-09 18:14:44)
> 
> 
> Review request for Default.
> 
> 
> Summary
> -------
> 
> CPU: Add functions to get the number of executed instructions and set the
> maximum number of instructions to execute to the CPUs and allow them to be
> called from python.
> 
> 
> Diffs
> -----
> 
>   src/cpu/BaseCPU.py 249f174e6f37 
>   src/cpu/base.hh 249f174e6f37 
>   src/cpu/base.cc 249f174e6f37 
>   src/cpu/o3/cpu.hh 249f174e6f37 
>   src/cpu/simple/base.hh 249f174e6f37 
>   src/python/swig/sim_object.i 249f174e6f37 
>   src/sim/eventq.hh 249f174e6f37 
>   src/sim/sim_object.hh 249f174e6f37 
>   src/sim/sim_object.cc 249f174e6f37 
> 
> Diff: http://reviews.m5sim.org/r/51/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Timothy
> 
>

_______________________________________________
m5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/m5-dev

Reply via email to