this sort of thing worries me because of the possibilities of introducing subtle bugs, but it looks like a good change long term.

src/sim/clocked_object.hh (Diff revision 2)

This obviously doesn't work if clock is not a constant... something like this would be more general (assuming I got it right):

  tick += divCeil(curTick() - tick, clock) * clock;

of course that's not a problem now, but it might be better to plan for that rather than having this be yet another thing that needs to be fixed later.

Show all issues

1. Andreas Hansson Aug. 2, 2012, 9:04 p.m. (Aug. 2, 2012, 9:04 p.m.)

   I'm not quite sure how that would solve the problem. The line above it is really the culprit, so this change on its own would not solve anything, as far as I can tell. Perhaps we can save the DVFS onslaught until we get there? :)

src/cpu/simple/timing.cc (Diff revision 2)

It looks like here you've converted a delay in ticks into a delay in cycles.

Show all issues

1. Andreas Hansson Aug. 2, 2012, 9:55 p.m. (Aug. 2, 2012, 9:55 p.m.)

   In the follow up patch they are now returning CycleCount to make this explicit. Will be posted in a min.

src/cpu/simple/timing.cc (Diff revision 2)

Same here.

Show all issues

src/sim/clocked_object.hh (Diff revision 2)

Sorry not to be more clear.  I meant that both lines 92 and 93 (the cycle and tick assignments) should be replaced.  The line I provided was meant to come first and show how tick would need to be updated based on its current value.  I was leaving the corresponding update of cycle as an exercise for the reader ;-).  Actually at that point I was probably in my phase where I was hoping that we could eliminate cycle entirely.

If we do keep cycle, it would be more like:

CycleCount elapsedCycles = divCeil(curTick() - tick, clock);
cycle += elapsedCycles;
tick += elapsedCycles * clock;

Show all issues

1. Andreas Hansson Aug. 7, 2012, 6:40 p.m. (Aug. 7, 2012, 6:40 p.m.)

   I agree conceptually. However, it actually changes the regression as the rounding is slightly different. This is not a problem as such, but I would still like to keep it out of this patch. Agreed?

2. Steve Reinhardt Aug. 8, 2012, 12:53 a.m. (Aug. 8, 2012, 12:53 a.m.)

   Weird... how does the rounding end up different?  They're all integers, so I'm confused about how that could happen.  It's early in the morning though so maybe I'm missing something obvious.
   
   If there is a good reason for the rounding to be different, then I am perfectly fine with leaving this change to another patch.  Actually I'm OK with leaving this change for later regardless, but mostly I'm stumped about why there's a difference.

3. Andreas Hansson Aug. 22, 2012, 9:17 a.m. (Aug. 22, 2012, 9:17 a.m.)

   After a lot of digging around I managed to finally get back to resurrecting this patch. 
   
   It turns out the reason for the difference I saw is not due to the rounding, but instead due to the shift from an absolute assignment of tick to a relative update...and the culprit is the fact that we already added tick += clock earlier in the update() method. Hence, moving the addition of clock into the if-statement and comparing tick + clock to curTick() fixes the issue of the difference I saw. I'll bump the patch.

4. Andreas Hansson Aug. 26, 2012, 1:09 a.m. (Aug. 26, 2012, 1:09 a.m.)

   Are you happy with this patch Steve?

Overall this looks great.  It would be nice if we could figure out how to make things like trapLatency below have their latencies depend on other clocks.  That way if you change the frequency of one clock all dependent clocks get updated automatically.  This is along the lines of what Joel was e-mailing about. 

src/arch/x86/mmapped_ipr.hh (Diff revision 4)

Why is the return value here 1 when the return type is still Tick?

Show all issues

1. Andreas Hansson Aug. 23, 2012, 2:04 a.m. (Aug. 23, 2012, 2:04 a.m.)

   This is fixed in the follow-on patch which introduces the Cycles type. I could make it uint64_t in the transition, but the two patches will be committed together.

2. Nathan Binkert Aug. 23, 2012, 2:14 a.m. (Aug. 23, 2012, 2:14 a.m.)

   ah, no worries then.

src/arch/x86/mmapped_ipr.hh (Diff revision 4)

ditto

Show all issues

src/cpu/inorder/cpu.cc (Diff revision 4)

This seems to be an absolute cycle.  Do we no longer require that cycles are relative?  We ensure that cycles are always increasing?

Show all issues

1. Andreas Hansson Aug. 23, 2012, 2:04 a.m. (Aug. 23, 2012, 2:04 a.m.)

   It is a bit of a blurry area, I agree. In this case it is used as an absolute cycle, but we only do that so that we later can subtract one absolute cycle from another and get a relative number. I am open to suggestions for how to solve the problem, but for now I think this is the best option.

2. Nathan Binkert Aug. 23, 2012, 2:17 a.m. (Aug. 23, 2012, 2:17 a.m.)

   This makes sense.  I think that we need to make it clear what an absolute number of cycles means.  i.e. that they truly have no fixed relationship to time, simply that they are a record of the number of clock cycles that have been triggered on a clocked object.  If we agree on that definition (perhaps with a comment on the type in the header file), then I think that Absolute Clocks are fine.

src/cpu/o3/O3CPU.py (Diff revision 4)

Be nice to have a Param type that specifically indicates Cycles, but I understand why not.

Show all issues

1. Andreas Hansson Aug. 23, 2012, 2:04 a.m. (Aug. 23, 2012, 2:04 a.m.)

   It is on the roadmap :)

src/cpu/o3/commit.hh (Diff revision 4)

Do we allow uint as a type?  Seems like we should use unsigned since we don't use uint elsewhere.

Show all issues

1. Andreas Hansson Aug. 23, 2012, 2:04 a.m. (Aug. 23, 2012, 2:04 a.m.)

   I will change to uint64_t. In the follow-on patch this will be Cycles (what was CycleCount)

2. Nathan Binkert Aug. 23, 2012, 2:14 a.m. (Aug. 23, 2012, 2:14 a.m.)

   no worries.

src/sim/clocked_object.hh (Diff revision 4)

I'd like these comments to be just a tad more informative.  How's this:

// The tick value of the next clock edge (>= curTick()) at the time of the last call to update()
mutable Tick tick;

// The cycle counter value corresponding to the current value of 'tick'
mutable Tick cycle;

Show all issues

src/sim/clocked_object.hh (Diff revision 4)

Since tick and clock correspond to the *next* clock edge, I think this '==' could be '>='.  It wouldn't change the results at all (I'm pretty sure) but would avoid the divide in more cases.

Also, re: your problem with tick += clock being outside the if -- I'd say the real problem is that between tick += clock and cycle += 1, one was outside the if and the other was inside.  So there's an alternate solution like this:

    tick += clock;
    cycle += 1;

    if (tick >= curTick()) {
        return;
    }

The elapsedCycles code doesn't have to change at all, though the value of elapsedCycles would now be smaller by one than the value with the current code.

I'm not sure which version is better; I thought of this new one in the process of trying to eliminate the redundant addition of tick to clock that's in the current code, though (1) I'd think a good compiler could eliminate that anyway and (2) it's just an integer add so who cares.  Feel free to pick whichever version you prefer.

Show all issues

1. Andreas Hansson Aug. 27, 2012, 1:56 a.m. (Aug. 27, 2012, 1:56 a.m.)

   The proposed change causes a lot of regressions to behave quite differently. I haven't managed to figure out exactly why yet, but intuitively I find my previous solution easier to follow.
   
   I'll do the changes one by one and see what the impact is (== to >= followed by moving the addition outside the if-statement).

2. Steve Reinhardt Aug. 27, 2012, 2:04 a.m. (Aug. 27, 2012, 2:04 a.m.)

   Interesting... do you agree that they look like they should be completely equivalent?
   
   Thanks for looking into this... I thought it would be a simple change, didn't mean to cause you a lot of extra work.

3. Andreas Hansson Aug. 27, 2012, 2:09 a.m. (Aug. 27, 2012, 2:09 a.m.)

   I also do not see why they are not equivalent. My guess is that it has to do with the case when the original tick < curTick() but tick + clock > curTick(), but that is just a guess. I'll do some more digging.

4. Andreas Hansson Aug. 27, 2012, 2:48 a.m. (Aug. 27, 2012, 2:48 a.m.)

   Indeed, it is exactly the case I mention that is causing problems with the code you suggested. With my original code, the if-statement will not hold (tick + clock is not curTick()) and so the update is based on the elapsedCycles.
   
   With your suggested change we have already added a clock period and a cycle, and actually we shouldn't have done so. I guess we can either add yet another if statement to catch this case, or fall back on the code as I had it previously.
   
   Ideas? Opinions? 

5. Steve Reinhardt Aug. 27, 2012, 3:14 a.m. (Aug. 27, 2012, 3:14 a.m.)

   At this point I'm mostly curious about why I was wrong when I thought that this code would be equivalent.  Your original code handles two special cases:
       tick == curTick()
       tick + clock == curTick()
   I'm trying to handle the additional case where
       tick < curTick() < tick + clock
   in which case the elapsedCycles calculation should end up having the same effect as
       { tick += clock; clock += 1; }
   because divCeil(curTick() - tick, clock) should be 1 if tick < curTick() <= tick + clock.  Right?
   
   So in other words if tick < curTick() then we should always be adding a clock period and a cycle, and the only question is whether adding one is sufficient or if we really need to add more than one.
   
   It just occured to me that the case that seems like it might be causing problems is if update() gets called with tick > curTick().  In this case I think what's happening with the current code is that both of the ifs fail, and we go through the elapsedCycles computation but curTick() - tick turns out to be negative so elapsedCycles is zero and nothing happens.
   
   So here's my new hypothesis: if you change the very first condition to;
       if (tick >= curTick())
           return;
   
   then all will be well, and we'll save even more trips through divCeil() than we even realized were happening before...
   
   Of course, I have been wrong before.
   
   
   

So my hypothesis was correct?  Nice... thanks for checking!

I'll just mention that there's a minor inconsistency in that one of the 'if (tick >= curTick())' checks has braces and the other doesn't.    Do what you will with that.  No need to repost.