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 6 commits into
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 |
1 similar comment
|
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
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
|
Hi @EweLo , thanks for this PR! I actually really need this feature for my thesis at CAPS now. |
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
…stener from MemHierarchy
… deprecated (old) constructor
69a5d22 to
21e37e0
Compare
|
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 I applied your suggested changes. Would you mind having a look at this again? :) |
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