> 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... > > Steve Reinhardt wrote: > 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 :). > > Andreas Hansson wrote: > Let's queue the issue of the request flags for now. > > What change do you suggest to this patch? I can add the IsPrefetch flag, > and use it for both SW and HW prefetches. Note though that we _never_ check > for this flag in the current code-base. > > Steve Reinhardt wrote: > I think you misunderstood; I'm not proposing to add an IsPrefetch flag. > I think I used "Prefetch" generically once when making a point that was > applicable to both SWPrefetch and HWPrefetch, but probably should have said > "*Prefetch" instead. It might be reasonable to have an IsPrefetch attribute > if we decide that it's appropriate to unify and replace SWPrefetch and > HWPrefetch, but that's as much a question about prefetch support as it is > about command attributes. We do check isSWPrefetch() in a few places, and it > may or may not be appropriate to fold HW prefetches into that behavior as > well. > > If you pare it back to just have the flush change, I would definitely > give that a 'ship it'. > > Andreas Hansson wrote: > I see. Your suggestion is to keep the IsSWPrefetch (and IsHWPrefetch) > attributes and methods, despite them only being used for a single command > type. I will nuke the isPrefetch method though since it is not used anywhere.
Yes. I think there's a pretty clear use case for IsSWPrefetch, even though we're not exploiting it yet. IsHWPrefetch is a little harder to justify, since we're not using it at all right now, but if we get rid of it, then HardPFReq would have the identical set of attributes as ReadReq, which doesn't seem right to me. Come to think of it, maybe that's the key difference in our approaches: I see the commands primarily as sets of attributes, with the command enum serving primarily as a compact encoding of a set of attributes via the lookup table, as opposed to the command enum all by itself having semantics. I suppose if I were feeling really rigid about that approach, I wouldn't want you to get rid of IsFlush either, so I'll stop thinking about it now. - 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 gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
