> On April 1, 2015, 4:28 p.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.
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. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2717/#review6005 ----------------------------------------------------------- On March 30, 2015, 9: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, 9: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
