src/cpu/o3/cpu.cc (Diff revision 1)

might want to update comment to say "cacheable block"

Show all issues

src/dev/dma_device.cc (Diff revision 1)

same here

Show all issues

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

is this really necessary?  I'm curious why setting this flag actually breaks things

Show all issues

1. Andreas Hansson April 1, 2015, 9:39 a.m. (April 1, 2015, 9:39 a.m.)

   We are not clearing it for the cache we are snooping into (see the block above), so it feels rather dodgy to pass the flag along here even if the recipient ignores it. Agreed?

2. Steve Reinhardt April 1, 2015, 9:51 a.m. (April 1, 2015, 9:51 a.m.)

   I guess I feel the opposite... what's the point in checking here to avoid setting the flag, if we know we're not going to bother looking at the value later?  This is like adding extra gates to clear a signal, when that signal is a don't care.  Now that you mention it, the same applies to the block above, where we are going out of our way to not call assertShared() even though (I assume) we're going to ignore that at the recipient as well.
   Basically all the snoop signals are conveying information, and while that information is not needed for uncached blocks, there's no harm in providing it either.  While we wouldn't be saving physical gates in this case, we would be avoiding a little irrelevant clutter in all these if conditions.

3. Andreas Hansson April 2, 2015, 2:40 a.m. (April 2, 2015, 2:40 a.m.)

   I will try removing them and see if any regression results change (when developing this I also had asserts in the packet ensuring that no uncacheable packet was ever marked as shared or having exclusive).
   My general view on this is that the additional if-statements help in understanding the code, and this is definitely soemthing the cache model needs more of :-). If the if-statements are removed I will at least expand on the comments and add a note that in the case of uncacheable we set the flag, but later ignore it.

4. Steve Reinhardt April 2, 2015, 7:36 a.m. (April 2, 2015, 7:36 a.m.)

   OK, thanks for giving it a try.
   We certainly don't disagree on the goal of making things more understandable, but seem to have different opinions on the means :).  To me, looking at this code, it makes me wonder why it's important that we not assert these signals for uncacheable blocks... is there some later point where we're going to look at the signal and, if it's set on an uncacheable block, bad things will happen?  So if the answer is "it's not important", then I think the reduction in clutter and complexity of not checking isUncachable() is the clearer option.
   I certainly would not object to comments clarifying the situation.  Maybe some uber-comment somewhere about how uncacheable blocks interact with coherence would be good.  This may be yet another case where the useful description should not only be in the commit message.

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

create a helper function for this loop, since it also appears below now?  or maybe we should convert AddrRangeList to a class, and add this as a method...

Show all issues

1. Andreas Hansson April 1, 2015, 9:39 a.m. (April 1, 2015, 9:39 a.m.)

   Sure, I'll make a little helper function

src/mem/cache/mshr.cc (Diff revision 1)

same comment as above

Show all issues

1. Andreas Hansson April 1, 2015, 9:39 a.m. (April 1, 2015, 9:39 a.m.)

   Same reason as above. It feels odd to set the flag that is later ignored.

src/mem/cache/mshr.cc (Diff revision 1)

why not:
if (!pkt->needsExclusive() && !pkt->req->isUncacheable()) {

Show all issues

Looks great!  Thanks for the changes!