Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Issue 3750 wrap up #4034
Issue 3750 wrap up #4034
Changes from 13 commits
ebaf14a
0a1c1f0
a5d8ad6
8d4a2e1
d66d9c6
9ddbe40
304c613
1ed82cd
1baf335
3c1e2f2
aa68680
90c05b8
d93defb
489ee64
2e5bfb9
cbbc4dc
8f98e51
7f2571c
57f5c17
e1775e3
6342468
c13c144
8bc38ae
f07d177
3563220
5f790b5
731f639
70dae97
c266541
2ab1b35
136fc2b
97beb28
e3c7a48
d3209ce
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 should be limited to one of MemorySlot or RegisterSlot.
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.
I haven't seen that restriction anywhere else, what does that stem from? I've seen tests with acquire taking memslot and reg slot. No one uses register slots yet, so it's possible they're not handled correctly in a couple places.
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.
Can users not use readout results for fast feedback and also get that result?
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.
It will be a practical restriction in the hardware. In reality it would be something like
Storing directly into a memory_slot is currently supported but is a convenience that masks hardware implementation. I would like to get away from multi-purpose instructions and I think this is a good first step that can be made while we are already changing the interface.
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.
so Acquire should really only take a register slot. This seems like an implementation change that is out of scope for this PR. Maybe next sprint?
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.
Will we be able to make the change before release?