> On Aug. 6, 2015, 9:51 p.m., Steve Reinhardt wrote: > > I confess to being confused by this patch---I thought the focus of the > > prior discussion was on all the flags in the Request object, but this > > change doesn't touch Request at all. > > > > Note that, unlike Request flags, MemCmd attributes have no storage > > overhead, as they're just enum values. So I don't think there's much > > savings in getting rid of them. They also enable more orthogonality in the > > cache code. While none of the removed attributes is currently used in more > > than one command, I think it's pretty easy to imagine that the Prefetch > > flags could get reused if we introduced an exclusive prefetch, or > > implemented the MMX non-temporal prefetch instructions, or some other > > variant form of prefetch. It might even be the case that we could > > implement an exclusive prefetch today just by defining a new command that > > has both IsSWPrefetch and NeedsExclusive set. (OK, it's pretty optimistic > > to think that would work out of the box, but that's the kind of thing this > > structure is supposed to enable.) > > > > Now the Request flags are another story in more ways than one... but I'll > > respond to that on the other thread. > > Andreas Hansson wrote: > This addresses the (5) item in my mail, not any of the other point. I > merely wanted to get the discussion going. > > The point here is not to save storage, but to _not_ convolute how we use > the attributes. We can add a "Prefetch" attribute, as you suggest, but no one > would currently be using it. Note that the cache checks for _software_ > prefetch request. If there is a need for a flag, I'd say let's add it when > the need arises. I am in no way against useful attributes. > > Really I wanted to show that there has been a bunch of cruft added based > on copy-paste, and before adding more of the same, I'd rather get to a point > where we agree on the usage (and document it :-). I am happy to scrap this > patch if there is disagreement. I wanted to highlight what is bugging me...
The flush change is perfectly fine with me; it does seem pretty unlikely that we'll have flavors of flush, or new operations that combine a flush with something else. The prefetch part I'm less comfortable with. It's certainly true that right now we're not using those attributes effectively, but I think that's as much a shortcoming of our caches as it is a justification for getting rid of the attribute. My point about the exclusive prefetch was serious: x86 has PREFETCHW and there are "prefetch with intent to write" instructions in AVX-512 as well. Assuming we do want to implement those someday, it seems obvious to me that a new command with both the IsSWPrefetch and NeedsExclusive attributes would be the way to do that. Do you disagree? If not, why bother to get rid of IsSWPrefetch now when it looks like we probably will want to add it back? I am not sure that there's a good reason to have separate IsSWPrefetch and IsHWPrefetch flags. Should we really be treating them differently? Maybe we should combine them into a single IsPrefetch flag. Also, although IsHWPrefetch is currently not used, I'm surprised; I thought there used to be code that would discard prefetches on MSHR full conditions. I'm not sure what happened to that. Popping up a level, I think I have two different reasons why I'm not enthusiastic about this: 1. While there is some clutter introduced, there's no other significant overhead, so it doesn't seem urgent to me. Furthermore, as we both know, a lot of additions are done based on copy-and-paste and following examples (which is indeed part of your complaint). Thus I am fairly confident that if we push this patch, then if someone else who has not followed this conversation decides to implement PREFETCHW, they will not say "hey, I should introduce an IsPrefetch attribute!", but instead they will effectively do s/tgt_pkt->cmd == MemCmd::SoftPFReq/tgt_pkt->cmd == MemCmd::SoftPFReq || tgt_pkt->cmd == MemCmd::SoftPFReqExcl/ and then we'll be relying on one or the other of us paying attention when that review is submitted to avoid seeing this get committed. So I think that erring on the side of introducing/keeping attributes even when we only think they might be useful in the future is a good policy. 2. I totally agree that the Request flags encoding is messy and inefficient; it started off with just a couple bits for Alpha and then grew from there. I also feel like, unless we really want to rearchitect both interfaces together, that that's an orthogonal issue. I don't mean to say they're totally independent---I'm all in favor of fixing up the Request interface to make it easier to generate Packets, for instance. But that doesn't imply a need to change the way Packets work, and this patch isn't really doing that anyway, it's just some minor tidying up. But in the big picture it feels to me like the patient has a broken wrist, and you're saying well why don't we do a manicure too while we're over here :). - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3004/#review6909 ----------------------------------------------------------- On Aug. 6, 2015, 2:41 p.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3004/ > ----------------------------------------------------------- > > (Updated Aug. 6, 2015, 2:41 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11004:e6aa2b4cfbef > --------------------------- > mem: Cleanup packet attributes and rely on command type > > This patch takes one step further in cleaning up the use of packet > command attributes, and shifts away from using attributes to > distinguish a single command. The commands affected are software > prefetch requests and flush requests. There is really no use for > attributes, as no other request packets share this property. > > Instead of looking at the attribute, the locations that identified > these packets now check for the actual command. > > > Diffs > ----- > > src/mem/cache/cache_impl.hh 4413195267fd > src/mem/packet.hh 4413195267fd > src/mem/packet.cc 4413195267fd > src/mem/ruby/system/DMASequencer.cc 4413195267fd > src/mem/ruby/system/RubyPort.cc 4413195267fd > src/mem/ruby/system/Sequencer.cc 4413195267fd > > Diff: http://reviews.gem5.org/r/3004/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
