Can you elaborate a bit on the functional issue for timing mode?  I can see where this makes a difference for atomic, but in timing mode arbitration between misses and writebacks is done in getNextMSHR() and isn't based on the order into which things are placed in mshrQueue vs. writeBuffer (except in the case where there are conflicts, which I don't think can happen here, since you wouldn't be writing back block X if you're making room for a request to block X).  Is this strictly related to uncacheable accesses?  Or is it really just an atomic-mode fix and you're changing the timing code to be consistent?

src/mem/cache/cache_impl.hh (Diff revision 1)

how about just creating a block here (from before this decl to after the loop) to localize the scope of 'writebacks', so we don't have to bother with the assert at the bottom?

Show all issues

1. Andreas Hansson March 23, 2015, 11:37 a.m. (March 23, 2015, 11:37 a.m.)

   Could do if you think it makes things prettier. I think I'd rather create a new variable with a different name if you think it is needed.

2. Steve Reinhardt March 23, 2015, 11:43 a.m. (March 23, 2015, 11:43 a.m.)

   I don't understand your response.  'writebacks' isn't used after the loop (afaict), so closing the scope at that point would not just make it evident that it's not used again, but enforce it at compile time.  I'm not sure what creating a new variable has to do with it, I assume because I'm just misunderstanding you.

3. Andreas Hansson March 23, 2015, 11:44 a.m. (March 23, 2015, 11:44 a.m.)

   I could also just drop the assert since it is not used after the while loop doing the writebacks. The assert is merely ensuring no one is adding any new items after that.

4. Andreas Hansson March 23, 2015, 11:46 a.m. (March 23, 2015, 11:46 a.m.)

   Ah, now I understand what you are saying. Sure, I can add a "{" and "}" around it. This would probably be the only place in the entire code base where we use this trick though.

5. Steve Reinhardt March 23, 2015, 11:57 a.m. (March 23, 2015, 11:57 a.m.)

   "Narrow your scope with this one weird trick!" I agree we don't use standalone blocks like this much, but I wouldn't consider it a trick :). I'm sure there are several places where we have standalone blocks within the cases of a switch, though you could argue that's just working around deficiencies in the scoping of switches.

6. Andreas Hansson March 23, 2015, noon (March 23, 2015, noon)

   Hehe, sounds good. I've updated the patch.
   It would be good to get all these "fixes" for the cache done and dusted before adding the locked RMW.

7. Steve Reinhardt March 23, 2015, 12:24 p.m. (March 23, 2015, 12:24 p.m.)

   Agreed.  I would like to push the more trivial patches (2688, 2689, 2690) ASAP to get them off my plate, but I'm happy to hold off on 2691 until these other things get in.

src/mem/cache/cache_impl.hh (Diff revision 1)

comment is wrong, we're issuing the writebacks (no write buffer in atomic mode)

Show all issues

1. Andreas Hansson March 23, 2015, 11:37 a.m. (March 23, 2015, 11:37 a.m.)

   Will fix.

src/mem/cache/cache_impl.hh (Diff revision 1)

looks like these were the only uses of writebackVisitor() and invalidateVisitor()... should we get rid of them?  TBH I missed when they were introduced.

Show all issues

1. Andreas Hansson March 23, 2015, 11:37 a.m. (March 23, 2015, 11:37 a.m.)

   We do memWriteback and memInvalidate when checkpointing and switching CPU models. Hence, they are still needed.

2. Steve Reinhardt March 23, 2015, 11:43 a.m. (March 23, 2015, 11:43 a.m.)

   Ah yes, I missed those uses.  Never mind.