-
Notifications
You must be signed in to change notification settings - Fork 90
Conversation
BitStates.hpp defined a custom implementation for tracking vectors of bits. However, `vector<bool>` from the STL already is a specialization of `vector` designed to save space and use a single bit for each index. Since we don't want to support the custom type in our API nor do we really need one internally, it was removed and all instances replaced with `vector<bool>`. Fixes #577
| this->lastUsedId--; | ||
| this->states.pop_back(); |
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.
Note that this does have a change in behavior here, where previously an allocated qubit that was released would never be used again, here we allow for qubit "reuse" by decrementing the lastUsedId member and shrinking the vector such that future allocations can use that id again. This will prevent the bit vector from always growing, such that Q# like this:
for i in 1..10 {
use q = Qubit();
X(q);
Reset(q);
}Will now have a bit vector of at most length 1 instead of length 10.
| void ReleaseQubit(Qubit qubit) override | ||
| { | ||
| const long id = GetQubitId(qubit); | ||
| assert(id <= this->lastUsedId); | ||
| assert(!this->states.IsBitSetAt(id)); | ||
| assert(!this->states.at(id)); | ||
| this->lastUsedId--; | ||
| this->states.pop_back(); | ||
| } |
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 have an impression that this function works correctly for the last-most qubit id only. But if we release a qubit "in the middle" then still the last-most qubit will be released. E.g. we call AllocateQubit() 3 times. It returns 0, 1, 2. Then we call ReleaseQubit(0), the fragment this->states.pop_back() removes the qubit with id 2 (rather than with id 0).
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.
Yes, that's correct, but in this case I'm relying on Q#'s strict qubit scoping for a shortcut. In Q#, every qubit has a scope, either implied or explicit, such that any qubit allocations are always released in the reverse order they were allocated. This does mean that the ToffoliSimulator could exhibit the bug you describe if it is used for QIR that was generated from something other than Q# (or written manually). However, because the simulators are packages as part of Microsoft.Quantum.Qir.QSharp.Core.dll it seems reasonable to restrict their behavior to Q# generated QIR. If someone wanted to use these simulators for another language, they'd have to conform to Q# gateset AND Q#'s lifetime semantics, which seems reasonable to me.
That said, this is a very subtle requirement that might not be obvious, and I'm open to further discussion. @bettinaheim, any concerns or suggestions about how to handle this?
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.
If the qubits are always deallocated in the reverse order of allocation then we are fine here. Can mark this conversation as resolved.
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.
In that case the parameter (Qubit qubit) becomes redundant and misleads:
void ReleaseQubit(Qubit qubit) override
kuzminrobin
left a comment
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 would like to know your opinion to my notes.
|
Great feedback, @kuzminrobin ! I've responded to the individual comments. |
BitStates.hpp defined a custom implementation for tracking vectors of bits. However,
vector<bool>from the STL already is a specialization ofvectordesigned to save space and use a single bit for each index. Since we don't want to support the custom type in our API nor do we really need one internally, it was removed and all instances replaced withvector<bool>.Fixes #577