Added the MemEvent Id to the CacheListenerNotification in the CacheListener from MemHierarchy#2622
Added the MemEvent Id to the CacheListenerNotification in the CacheListener from MemHierarchy#2622EweLo wants to merge 4 commits intosstsimulator:develfrom
Conversation
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
@gvoskuilen would you be willing to review this? :) |
gvoskuilen
left a comment
gvoskuilen
left a comment
There was a problem hiding this comment.
This change is sounds OK in general but I'd ask for two changes: (1) use SST::Event::id_type and SST::Event::NO_ID as the type is defined in that class, not memHierarchy and (2) ensure that event_id is not a required field since this interface is used by non-memHierarchy libraries and requiring it will break them.
| /* Listener callbacks */ | ||
| virtual void notifyListenerOfAccess(MemEvent * event, NotifyAccessType access_type, NotifyResultType result_type); | ||
| virtual void notifyListenerOfEvict(Addr addr, uint32_t size, uint64_t ip); | ||
| virtual void notifyListenerOfEvict(Addr addr, uint32_t size, Addr ip, MemEventBase::id_type evId); |
There was a problem hiding this comment.
id_type is actually defined in the SST::Event class. Can you update the references to it to point to that class instead?
| mshr_->insertWriteback(tag->getAddr(), false); | ||
| recordPrefetchResult(tag, stat_prefetch_evict_); | ||
| notifyListenerOfEvict(data->getAddr(), line_size_, 0); | ||
| notifyListenerOfEvict(data->getAddr(), line_size_, 0, MemEventBase::NO_ID); |
There was a problem hiding this comment.
Should be SST::Event::NO_ID
| mshr_->insertWriteback(tag->getAddr(), false); | ||
| recordPrefetchResult(tag, stat_prefetch_evict_); | ||
| notifyListenerOfEvict(data->getAddr(), line_size_, 0); | ||
| notifyListenerOfEvict(data->getAddr(), line_size_, 0, MemEventBase::NO_ID); |
| mshr_->insertWriteback(tag->getAddr(), false); | ||
| recordPrefetchResult(tag, stat_prefetch_evict_); | ||
| notifyListenerOfEvict(data->getAddr(), line_size_, 0); | ||
| notifyListenerOfEvict(data->getAddr(), line_size_, 0, MemEventBase::NO_ID); |
| NotifyAccessType accessT, | ||
| NotifyResultType resultT) : | ||
| NotifyResultType resultT, | ||
| const MemEventBase::id_type evId) : |
There was a problem hiding this comment.
Can you also ensure there is still a constructor that doesn't require the id_type field/give id a default value? Other non-memHierarchy libraries use this interface and will break otherwise.
Also change MemEventBase::id_type to SST::Event::id_type
| NotifyAccessType getAccessType() const { return access; } | ||
| NotifyResultType getResultType() const { return result; } | ||
| uint32_t getSize() const { return size; } | ||
| MemEventBase::id_type getEventID() const { return eventId; } |
| Addr instPtr; | ||
| NotifyAccessType access; | ||
| NotifyResultType result; | ||
| MemEventBase::id_type eventId; |
There was a problem hiding this comment.
SST::Event::id_type and give it a default value of SST::Event::NO_ID
3 participants
This PR extends the
CacheListenerNotificationinterface to include the ID of theMemEventthat triggered the listener callback, enabling custom CacheListener implementations to trace the corresponding MemEvents.I implemented this because I'm working on a complex tracer in SST that tracks memory access patterns and I therefore need to be able to correlate CacheListener notifications with specific memory events.
eventIdfield andgetEventID()getter toCacheListenerNotificationnotifyAccess()calls across MemHierarchy components to pass the event ID