> On April 1, 2015, 9:28 a.m., Steve Reinhardt wrote: > > src/mem/cache/cache_impl.hh, line 1724 > > <http://reviews.gem5.org/r/2717/diff/1/?file=44386#file44386line1724> > > > > is this really necessary? I'm curious why setting this flag actually > > breaks things > > Andreas Hansson wrote: > 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? > > Steve Reinhardt wrote: > 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. > > Andreas Hansson wrote: > 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.
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. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2717/#review6005 ----------------------------------------------------------- On March 30, 2015, 2:17 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2717/ > ----------------------------------------------------------- > > (Updated March 30, 2015, 2:17 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10783:84330bcae3c6 > --------------------------- > mem: Snoop into caches on uncacheable accesses > > This patch takes a last step in fixing issues related to uncacheable > accesses. We do not separate uncacheable memory from uncacheable > devices, and in cases where it is really memory, there are valid > scenarios where we need to snoop since we do not support cache > maintenance instructions (yet). On snooping an uncacheable access we > thus provide data if possible. In essence this makes uncacheable > accesses IO coherent. > > The snoop filter is also queried to steer the snoops, but not updated > since the uncacheable accesses do not allocate a block. > > At the moment there are quite a few special cases where we > need to check to ensure uncacheable accesses do not steal exclusivity > etc, and perhaps there is a nicer way to embed these checks in the > existing packet methods. > > > Diffs > ----- > > src/mem/cache/mshr.cc 8a7285d6197e > src/mem/coherent_xbar.cc 8a7285d6197e > src/mem/snoop_filter.cc 8a7285d6197e > src/cpu/o3/cpu.cc 8a7285d6197e > src/dev/dma_device.cc 8a7285d6197e > src/mem/cache/cache_impl.hh 8a7285d6197e > > Diff: http://reviews.gem5.org/r/2717/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
