This seems like a great idea.  I'm a bit concerned that there is something still lurking.  Are we sure we know what the issue used to be and that it'd no longer a problem?

src/base/types.hh (Diff revision 1)

Are we sure that this is no longer the case?

Show all issues

1. Andreas Hansson July 25, 2012, 11:50 p.m. (July 25, 2012, 11:50 p.m.)

   All the regressions work :), if someone knows what the issue was I'd be keen to know more.

2. Steve Reinhardt July 26, 2012, 1:19 a.m. (July 26, 2012, 1:19 a.m.)

   I don't know for sure, but it could be a matter of doing a subtraction and checking for a negative result... however this code here looks safe, so maybe that's been taken care of:
   http://repo.gem5.org/gem5/file/b57966a6c512/src/mem/cache/tags/lru.cc#l130

3. Andreas Hansson July 26, 2012, 1:33 a.m. (July 26, 2012, 1:33 a.m.)

   If there is a specific test to run let me know. As mentioned, all regressions pass.

4. Steve Reinhardt July 26, 2012, 1:44 a.m. (July 26, 2012, 1:44 a.m.)

   Sorry not to be clearer... my point was that I don't see any code that should have a problem with unsigned ticks.  My hypothesis is that at some point in the past this code I'm pointing to:
   
      if (blk->whenReady > curTick()
          && blk->whenReady - curTick() > hitLatency) {
          lat = blk->whenReady - curTick();
      }
   
   may have been written something more like:
   
       Tick delay = blk->whenReady - curTick();
       lat = (delay < 0) ? 0 : delay;
   
   which works fine as long as Tick is signed but breaks when it's unsigned.  But since I don't see any code like that right now, I'm cautiously optimistic that this comment is obsolete.
   

5. Andreas Hansson July 26, 2012, 2:19 a.m. (July 26, 2012, 2:19 a.m.)

   It would be good to remove the "-Wno-sign-compare", but I am afraid that would take quite some effort.

6. Andreas Hansson July 28, 2012, 1:45 a.m. (July 28, 2012, 1:45 a.m.)

   Are we sure enough by now, or still keen to have some more input in addition to the regressions?

7. Ali Saidi July 28, 2012, 3:28 a.m. (July 28, 2012, 3:28 a.m.)

   I'm fine... we should just remember if we experience a bug in the next few weeks that this might be the cause...

src/base/types.hh (Diff revision 1)

Is this the correct value of maxtick now?  Shouldn't it be ULL instead of LL?

Show all issues

src/sim/eventq.hh (Diff revision 1)

Seems like the comment no longer applies

Show all issues

src/sim/eventq.hh (Diff revision 1)

This too

Show all issues

src/base/types.hh (Diff revision 2)

This is the first time I have seen ULL being written in this fashion. I usually write the number followed by ULL. For example - 123ULL.

1. Andreas Hansson July 26, 2012, 1:14 a.m. (July 26, 2012, 1:14 a.m.)

   I was merely sticking with the format that was used before. I'm not against changing it if desired.

2. Ali Saidi July 26, 2012, 1:15 a.m. (July 26, 2012, 1:15 a.m.)

   ULL() is a macro we have that does 0x1123123ULL. It was a added increase we needed to do something else for a different compiler, but it never seemed to be required. 
   
   

I agree with Ali... looks like it's OK, but it's good to keep this particular change in mind if someone reports a bug in the future.