Hi Victor,
Thank you for your explanation. I admit such separate design style (or
idea) between cache and prefetcher is rather flexible. Cache just notifies
the prefetcher of the access events, but doesn't make decision on whether a
prefetch will be issued for the prefetcher.
However, will doing so lose some performance? After all,
BasePrefetcher::observeAccess() will call inCache() (it will call
tags->findblock()) and inMissQueue (it will call mshrQueue.findMatch()). If
the application is memory-intensive one, the overhead of this part should
be very high.
In addition, do you think whether the call of inMissQueue is necessary or
not in observeAccess()?
(The key code is:
if(onMiss) {
return !inCache(addr, is_secure) && !inMissQueue(addr, is_secure);
}
)
This call looks like a little weird. If we want to issue a prefetch request
only when a miss occurs, this missed req must have been added into the
MSHR. So now, how can we require !inMissQueue is TRUE (not in MSHR). That
sounds like a contradiction, right? Actually I can't observe any prefetch
events until I comment out the "&& !inMissQueue(addr, is_secure);" (I use
stride prefetcher for l1 dcache). So I think this condition judgement is a
little abnormal (weird ? too strict? I'm not sure).
Also,I understand that your prefetching idea is that only when there are no
existing outstanding demand misses, the prefetch requests can be issued
from pfq. But I think we can issue the prefetch as long as we have avaiable
mshr entries, otherwise, the prfetch requests will have rare opportunities
to be issued.
Another thing, I noticed that there are some unnecessary tag check in the
code about cache.
1 Cache::recvTimingResp
-----------------------------------------
MSHR::Target *initial_tgt = mshr->getTarget();
CacheBlk *blk = tags->findBlock(pkt->getAddr(), pkt->isSecure());
Note: here blk must be NULL. (I add assert(blk==NULL) to verify this
thing.) Because pkt here is a received response packet, so it means that no
previous hit for the addr in this packet happens (it should be a cache
miss). And the pkt->senderState contains the corresponding mshr. So it
definitely should not in the cache. According to this analysis, why do we
still need to check the tags(tags->findBlock())?
-----------------------------------------
2 Cache::getTimingPacket()
-----------------------------------------------------------------------------------------
else {
CacheBlk *blk = tags->findBlock(mshr->blkAddr, mshr->isSecure);
ditto: here blk must also be NULL because it tries to find a adder from
MSHR. All addrs from MSHR should not be found in the cache, right? (I add
assert(blk==NULL) to verify this thing.)
------------------------------------------------------------------------------------------
3 Cache::recvTimingReq
-------------------------------------------------------------------
else {
if (blk && blk->isValid()) {
// should have flushed and have no valid block
assert(!pkt->req->isUncacheable());
...
I tested several applications and found that blk is always NULL. So why
do we have a if(...) here?
Finally, about patch, I haven't learned how to submit a patch but I'll see
the related instructions and try to do so if possible.
Thank you again
gjins
On Thu, Feb 4, 2016 at 2:37 AM, Victor Garcia <[email protected]> wrote:
> Hi gjins,
>
> That comment about "calling" the prefetcher means that the cache notifies
> the prefetcher of every request it sees. It is then up to each prefetcher
> to decide whether to use that access to train or not, based on the policy
> it is configured with.
>
> On BasePrefetcher::observeAccess() the prefetcher checks whether it has to
> observe that request or not, so if your prefetcher is configured to
> prefetch only on misses, the cache will also notify it of hits, but it will
> then return false in the observeAccess function and do nothing with them.
>
> Regarding your bug report, at first glance it seems you are right. I
> suggest you make a patch with the fix and submit it:
> http://www.m5sim.org/Submitting_Contributions
>
> Víctor
>
> On Wed, Feb 3, 2016 at 8:05 PM, Gongjin Sun <[email protected]> wrote:
>
>> Hi all and maintainer,,
>>
>> I have some confusion and a possible bug report about prefetcher. The
>> report is in the second half of the following description.
>>
>> Confusion:
>>
>> I noticed that there is a comment when the prefetcher->notify is call. It
>> is in the "Cache::recvTimingReq" and says:
>>
>> "*// We should call the prefetcher reguardless if the request is*
>> *// satisfied or not, reguardless if the request is in the MSHR or*
>> *// not. The request could be a ReadReq hit, but still not*
>> *// satisfied (potentially because of a prior write to the same*
>> *// cache line. So, even when not satisfied, tehre is an MSHR*
>> *// already allocated for this, we need to let the prefetcher know*
>> *// about the request*
>> if (prefetcher) {
>> // Don't notify on SWPrefetch
>> if (!pkt->cmd.isSWPrefetch())
>> next_pf_time = prefetcher->notify(pkt);
>> }
>> "
>> I'm a little confused with these.
>> As far as I know, if we configure that a prefetch request should issued
>> when a cache miss occurs, we should call the prefetcher ONLY when the
>> request is not satisfied and this request is not in MSHR, right? If
>> satisfied, the cache will fetch the right data. If the request is already
>> in MSHR, it can come from the previous outstanding demand request or
>> prefetch request which are still in service, so we shouldn't call the
>> prefetcher. So here I really don't understand this statement: "* reguardless
>> if the request is*
>> *// satisfied or not, reguardless if the request is in the MSHR or*
>> *// not."*
>>
>> In addition, about "*because of a prior write to the same **cache
>> line.", *it looks like a RAW for the same data, and the previous write
>> has not finished, so the subsequent read (that is, current read request)
>> can't continue and can't be satisfied. That means that previous write
>> request encountered a miss but hasn't allocated its new data line. However,
>> the comment mentions that current request could be a ReadReq hit (it means
>> ReadReq found this line). See, it is a contradiction.
>>
>> Also about "*we need to let the prefetcher know **about the request"*,
>> does that mean "we need to let the prefetcher check whether the request is
>> in the MSHR or not"?
>>
>> A possible bug:
>>
>> Please see the following code (in Cache::recvTimignReq):
>> *" allocateMissBuffer(pkt, forward_time, true);*
>> * }*
>>
>> * if (prefetcher) {*
>> * // Don't notify on SWPrefetch*
>> * if (!pkt->cmd.isSWPrefetch())*
>> * next_pf_time = prefetcher->notify(pkt);*
>> * }*
>> * }*
>> *"*
>> if pkt's request was allocated an entry in *allocateMissBuffer, *it
>> should be found in the following prefetcher->notify() in which
>> Cache::inMissQueue() is called. However, when I debug this code, I found it
>> is not the case. The notify() can't detect the just inserted MSHR entry.
>> After reading lots of code, I found its reason.
>>
>> The allocateMissBuffer(...)'s implementation is:
>>
>>
>> -------------------------------------------------------------------------------
>> MSHR *allocateMissBuffer(PacketPtr pkt, Tick time, bool requestBus)
>> {
>> return allocateBufferInternal(&mshrQueue,
>> *blockAlign(pkt->getAddr())*, blkSize,
>> pkt, time, requestBus);
>> }
>>
>> ---------------------------------------------------------------------------------------
>> It use the addr "*blockAlign(pkt->getAddr())" *when inserting a request
>> packet request.
>>
>> But in the call chain "prefetcher->nofity() ==> observeAccess() ==>
>> inMissQueue() ==>Cache->insMissQueue()",
>>
>>
>> ----------------------------------------------------------------------------------------
>> the Cache::inMissQueue()'s implementation is:
>> bool inMissQueue(Addr *addr*, bool is_secure) const {
>> return (mshrQueue.findMatch(addr, is_secure) != 0);
>> }
>>
>> ---------------------------------------------------------------------------------------------
>>
>> See, the inMissQueue uses *addr* (that is, pkt->getAddr()) rather than
>> *blockAlign(addr)*. The key point is here. After I add the blockAlign,
>> the notify can detect the just inserted MSHR entry.
>>
>> The version I use is: gem5-stable-629fe6e6c781.tar.bz2
>>
>> I'm not sure if this is a bug. So please verify it. Thank you.
>>
>> gjins
>>
>>
>>
>>
>>
>>
>> _______________________________________________
>> gem5-users mailing list
>> [email protected]
>> http://m5sim.org/cgi-bin/mailman/listinfo/gem5-users
>>
>
>
> _______________________________________________
> gem5-users mailing list
> [email protected]
> http://m5sim.org/cgi-bin/mailman/listinfo/gem5-users
>
_______________________________________________
gem5-users mailing list
[email protected]
http://m5sim.org/cgi-bin/mailman/listinfo/gem5-users
