Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Remove BitStates.hpp #581

Merged
swernli merged 5 commits into from
Mar 24, 2021
Merged

Remove BitStates.hpp #581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3bfaaa0
Remove BitStates.hpp
swernli Mar 23, 2021
a1de729
CR feedback
swernli Mar 23, 2021
f68b6ff
Merge branch 'main' into swernli/qir-cleanup
swernli Mar 24, 2021
34fd856
Merge branch 'main' into swernli/qir-cleanup
swernli Mar 24, 2021
8aaddf9
Merge branch 'main' into swernli/qir-cleanup
swernli Mar 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions src/QirRuntime/lib/Simulators/ToffoliSimulator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
#include "QSharpSimApi_I.hpp"
#include "SimFactory.hpp"

#include "BitStates.hpp"

namespace Microsoft
{
namespace Quantum
Expand All @@ -24,7 +22,7 @@ namespace Quantum

// State of a qubit is represented by a bit in states indexed by qubit's id,
// bits 0 and 1 correspond to |0> and |1> states respectively.
BitStates states;
std::vector<bool> states;

// The clients should never attempt to derefenece the Result, so we'll use fake
// pointers to avoid allocation and deallocation.
Expand Down Expand Up @@ -67,21 +65,23 @@ namespace Quantum
Qubit AllocateQubit() override
{
this->lastUsedId++;
this->states.ExtendToInclude(this->lastUsedId);
this->states.emplace_back(false);
return reinterpret_cast<Qubit>(this->lastUsedId);
}

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();
Comment on lines +77 to +78
Copy link
Collaborator Author

@swernli swernli Mar 23, 2021 •
edited
Loading

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.

}
Comment on lines 72 to 79
Copy link
Contributor

@kuzminrobin kuzminrobin Mar 23, 2021

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).

Copy link
Collaborator Author

@swernli swernli Mar 23, 2021

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?

Copy link
Contributor

@kuzminrobin kuzminrobin Mar 24, 2021

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.

Copy link
Contributor

@kuzminrobin kuzminrobin Mar 24, 2021

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


std::string QubitToString(Qubit qubit) override
{
const long id = GetQubitId(qubit);
return std::to_string(id) + ":" + (this->states.IsBitSetAt(id) ? "1" : "0");
return std::to_string(id) + ":" + (this->states.at(id) ? "1" : "0");
}

///
Expand Down Expand Up @@ -120,15 +120,15 @@ namespace Quantum
///
void X(Qubit qubit) override
{
this->states.FlipBitAt(GetQubitId(qubit));
this->states.at(GetQubitId(qubit)).flip();
}

void ControlledX(long numControls, Qubit* const controls, Qubit qubit) override
{
bool allControlsSet = true;
for (long i = 0; i < numControls; i++)
{
if (!this->states.IsBitSetAt(GetQubitId(controls[i])))
if (!this->states.at(GetQubitId(controls[i])))
{
allControlsSet = false;
break;
Expand All @@ -137,7 +137,7 @@ namespace Quantum

if (allControlsSet)
{
this->states.FlipBitAt(GetQubitId(qubit));
this->states.at(GetQubitId(qubit)).flip();
}
}

Expand All @@ -153,7 +153,7 @@ namespace Quantum
}
if (bases[i] == PauliId_Z)
{
odd ^= (this->states.IsBitSetAt(GetQubitId(targets[i])));
odd ^= (this->states.at(GetQubitId(targets[i])));
}
}
return odd ? one : zero;
Expand Down
83 changes: 0 additions & 83 deletions src/QirRuntime/public/BitStates.hpp
View file
Open in desktop

This file was deleted.

48 changes: 31 additions & 17 deletions src/QirRuntime/test/unittests/QirRuntimeTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,34 +14,33 @@
#include "qsharp__core__qis.hpp"
#include "QirRuntime.hpp"

#include "BitStates.hpp"
#include "QirContext.hpp"
#include "SimulatorStub.hpp"

using namespace Microsoft::Quantum;

struct ResultsReferenceCountingTestQAPI : public SimulatorStub
{
int lastId = 1;
int lastId = -1;
const int maxResults;
BitStates allocated;
std::vector<bool> allocated;

static int GetResultId(Result r)
{
return static_cast<int>(reinterpret_cast<int64_t>(r));
}

ResultsReferenceCountingTestQAPI(int maxResults)
: maxResults(maxResults + 2)
: maxResults(maxResults),
allocated(maxResults, false)
{
allocated.ExtendToInclude(maxResults);
}

Result Measure(long, PauliId[], long, Qubit[]) override
{
assert(this->lastId < this->maxResults);
this->lastId++;
this->allocated.SetBitAt(this->lastId);
this->allocated.at(this->lastId) = true;;
return reinterpret_cast<Result>(this->lastId);
}
Result UseZero() override
Expand All @@ -56,8 +55,8 @@ struct ResultsReferenceCountingTestQAPI : public SimulatorStub
{
const int id = GetResultId(result);
INFO(id);
REQUIRE(this->allocated.IsBitSetAt(id));
this->allocated.FlipBitAt(id);
REQUIRE(this->allocated.at(id));
this->allocated.at(id).flip();
}
bool AreEqualResults(Result r1, Result r2) override
{
Expand All @@ -66,7 +65,14 @@ struct ResultsReferenceCountingTestQAPI : public SimulatorStub

bool HaveResultsInFlight() const
{
return this->allocated.IsAny();
for (const auto& b : this->allocated)
{
if (b)
{
return true;
}
}
return false;
}
};
TEST_CASE("Results: comparison and reference counting", "[qir_support]")
Expand Down Expand Up @@ -693,31 +699,32 @@ struct QubitTestQAPI : public SimulatorStub
{
int lastId = -1;
const int maxQubits;
BitStates allocated;
std::vector<bool> allocated;

static int GetQubitId(Qubit q)
{
return static_cast<int>(reinterpret_cast<int64_t>(q));
}

QubitTestQAPI(int maxQubits)
: maxQubits(maxQubits)
: maxQubits(maxQubits),
allocated(maxQubits, false)
{
allocated.ExtendToInclude(maxQubits);
}

Qubit AllocateQubit() override
{
assert(this->lastId < this->maxQubits);
this->lastId++;
this->allocated.SetBitAt(this->lastId);
this->allocated.at(this->lastId) = true;
return reinterpret_cast<Qubit>(this->lastId);
}
void ReleaseQubit(Qubit qubit) override
{
const int id = GetQubitId(qubit);
INFO(id);
REQUIRE(this->allocated.IsBitSetAt(id));
this->allocated.FlipBitAt(id);
REQUIRE(this->allocated.at(id));
this->allocated.at(id).flip();
}
std::string QubitToString(Qubit qubit) override
{
Expand All @@ -735,12 +742,19 @@ struct QubitTestQAPI : public SimulatorStub

bool HaveQubitsInFlight() const
{
return this->allocated.IsAny();
for (const auto& b : this->allocated)
{
if (b)
{
return true;
}
}
return false;
}
};
TEST_CASE("Qubits: allocate, release, dump", "[qir_support]")
{
std::unique_ptr<QubitTestQAPI> qapi = std::make_unique<QubitTestQAPI>(3);
std::unique_ptr<QubitTestQAPI> qapi = std::make_unique<QubitTestQAPI>(4);
QirContextScope qirctx(qapi.get());
QirString* qstr = nullptr;

Expand Down