> On Aug. 7, 2015, 4:51 a.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. > > Steve Reinhardt wrote: > 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.
I see your point. Perhaps the better long-term approach is to make cmd private, and always rely on the attributes (which then would have to be more elaborate)...let's give this some thought. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3004/#review6909 ----------------------------------------------------------- On Aug. 6, 2015, 9: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, 9: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
