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:


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.


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 :).

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.

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'.

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.

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.

That's right, my initial vision was that all the coherence logic would only test the attributes.  The command names are handy for legibility and for keeping the representation compact.