-
Notifications
You must be signed in to change notification settings - Fork 46
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
Move bank instrumentation to bank.{read/write} #227
base: main
Are you sure you want to change the base?
Move bank instrumentation to bank.{read/write} #227
Conversation
This makes sure instrumentation works also for device-internal accesses, which allows a clean migration of sideband banks to the transaction interface. This also moves the feature further away from the memop, which means:
|
Verification #12409052: fail |
90cdf54
to
76fdecd
Compare
Verification #12409305: pass |
Jenkins looks ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat surprised this shuffling of calls to the _callback
family doesn't break anything.
You don't think VP passing is mostly due to their own local copy-pasting of these methods?
VP passing is probably due to their tests not relying much on bank instrumentation (in part because bank instrumentation don't work for sideband accesses before this PR). I think their copied overrides will cause callbacks to be called twice, but things that do rely on bank instrumentation are likely on the form "wait for next access", where a double callback probably isn't disruptive. |
Here is the issue for fixing this in VP. |
Before merging this, I should also add a release note. |
I can see that the VP duplication issue is scheduled for fixing before nov 17, so we can consider waiting for that. |
76fdecd
to
1ec9514
Compare
that if <tt>read</tt> or <tt>write</tt> is overridden in a bank, | ||
then instrumentation callbacks will only be called if they | ||
call <tt>default()</tt>.</add-note></build-id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This perhaps deserves a discussion, there are probably valid use cases for read/write overrides where this change will be a regression. I don't know what to do about that.
Verification #12413771: pass |
No description provided.