Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dv] Add spurious responses to memory agent #2185

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

GregAC
Copy link
Collaborator

@GregAC GregAC commented Jul 2, 2024

A spurious response is one that isn't associated with any on-going request. With this new feature the memory agent can generate them randomly when the interface is idle (i.e. there are no outstanding requests).

@GregAC
Copy link
Collaborator Author

GregAC commented Jul 2, 2024

I still need to do a full regression with this version but regressions with earlier prototypes were working.


while(!(vif.monitor_cb.request && vif.monitor_cb.grant)) begin

if (vif.monitor_cb.rvalid && !vif.monitor_cb.spurious_response) begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The !spurious_response bit seems slightly surprising here. Should the monitor really care about whether the response was the one we were waiting for? I guess the problem is the outstanding_accesses counter, but maybe that really belongs in the driver or the sequence rather than the monitor, since those components know whether they have anything "in flight".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is partially down to laziness on my part but pre this change spurious responses didn't exist and downstream users of the monitor (i.e. those things connecting to the item_collected_port) were expecting everything they receive to represent an actual transaction that occurred (in particular the co-sim scoreboard). If we output spurious responses here we'll need all downstream users of the monitor to filter out spurious responses. This felt like a cleaner design.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that feels reasonable. Maybe an example of where "the principled solution" is also the wrong one :-)

@GregAC GregAC force-pushed the spurious_responses branch from 0858603 to 9dace64 Compare July 3, 2024 19:58
@GregAC GregAC requested a review from rswarbrick July 3, 2024 19:59
A spurious response is one that isn't associated with any on-going
request. With this new feature the memory agent can generate them
randomly when the interface is idle (i.e. there are no outstanding
requests).
@GregAC GregAC force-pushed the spurious_responses branch from 9dace64 to ae13c5f Compare July 3, 2024 20:06
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the responses. This looks sensible to me.

@GregAC GregAC added this pull request to the merge queue Jul 4, 2024
Merged via the queue into lowRISC:master with commit 6682336 Jul 4, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants