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

T anikda/experimental #291

Merged
AniketDalvi merged 15 commits into from
Jul 30, 2020
Merged

T anikda/experimental #291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e32e0b2
Initial wfn permute changes
Jun 16, 2020
f48134e
changes to reorder to fix minor bugs
Jun 16, 2020
3281075
rebase with clean sim branch
Jun 17, 2020
c3189fb
Reordering wfn to work with existing flush/fuse logic
Jun 25, 2020
4beea14
resolving conflicts
Jun 25, 2020
d6b2ca2
Added some comments
Jul 1, 2020
91eef28
Merge branch 'master' of https://github.com/microsoft/qsharp-runtime …
Jul 1, 2020
945fb33
Merge branch 'feature/simulator-enhancement' of https://github.com/mi…
Jul 1, 2020
1c4b977
Combining Dave's scheduling logic with reordering
Jul 3, 2020
97bd17c
PR changes
Jul 22, 2020
1c4b04f
PR changes
Jul 22, 2020
07bbb87
PR changes
Jul 25, 2020
d91fab3
PR change to avoid copying
Jul 28, 2020
8f3c963
PR changes
Jul 28, 2020
bcb37db
PR changes to undo character change in Haner
Jul 28, 2020
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
28 changes: 28 additions & 0 deletions src/Simulation/Native/CMakeSettings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{
"configurations": [
{
"name": "x64-Debug",
"generator": "Ninja",
"configurationType": "Debug",
"inheritEnvironments": [ "msvc_x64_x64" ],
"buildRoot": "${projectDir}\\out\\build\\${name}",
"installRoot": "${projectDir}\\out\\install\\${name}",
"cmakeCommandArgs": "",
"buildCommandArgs": "",
"ctestCommandArgs": "",
"variables": []
},
{
"name": "x64-Release",
"generator": "Ninja",
"configurationType": "RelWithDebInfo",
"buildRoot": "${projectDir}\\out\\build\\${name}",
"installRoot": "${projectDir}\\out\\install\\${name}",
"cmakeCommandArgs": "",
"buildCommandArgs": "",
"ctestCommandArgs": "",
"inheritEnvironments": [ "msvc_x64_x64" ],
"variables": []
}
]
}
48 changes: 26 additions & 22 deletions src/Simulation/Native/src/external/fused.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,14 @@ class Fused
fusedgates = Fusion();
}

const Fusion& get_fusedgates() const {
return fusedgates;
}

void set_fusedgates(Fusion newFusedGates) const {
Copy link
Contributor

@IrinaYatsenko IrinaYatsenko Jul 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Declaring setters as const doesn't make sense: the purpose of the setter is to change the object, isn't it?

Another problem with the signature is that it still does an extra unnecessary copy of Fusion object, first into the parameter when the function is invoked and then once again from the parameter into fusedgates. Change it to void set_fusedgates(const Fusion& newFusedGates) { fusedgates = newFusedGates; }

Copy link
Contributor Author

@AniketDalvi AniketDalvi Jul 23, 2020

Choose a reason for hiding this comment

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

This needs to be set as const, as I am calling it in flush() which is const. Yes, the setter is supposed to change the object - but in this case by change here we are changing the contents of the container, not the container or its location.

Copy link
Contributor

@IrinaYatsenko IrinaYatsenko Jul 23, 2020

Choose a reason for hiding this comment

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

As a part of public class interface a const setter doesn't make much sense, whatever the implementation details of other methods might be. If clients outside of the class need to use the setter (for example, tests), make it non-const for them and inside flush() do the assignment directly, without going through the setter.

Copy link
Contributor Author

@AniketDalvi AniketDalvi Jul 24, 2020

Choose a reason for hiding this comment

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

Apologies, I did not really understand what you were trying to convey here completely. Unfortunately because the data structure fusedgates is private, I cannot make the assignments outside this class, thereby not allowing me to make assignments in flush() directly. Given this, the use of a setter is necessary to alter the datastructure. And as this setter is being called in a function which is declared const, the language does not allow me to have the setter to be non-const.

Copy link
Contributor

@IrinaYatsenko IrinaYatsenko Jul 24, 2020

Choose a reason for hiding this comment

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

"funny" public interfaces like this usually indicate that there is an issue with design of data structures: basically, you are trying to put a square peg into a round hole. You have multiple choices to address it:

  1. keep entrenching the previous design decision(s) and add public interfaces that don't conform to good practices (ugly)
  2. keep the previous design decision intact and workaround by adding const_cast(s) (ugly but obvious to spot in code and keeps badness from spreading)
  3. remove const from member functions that violate constness (easy, as any chickening out is, but honest :))
  4. rethink the logical\representational constness model of your data structures and get it right (hard and, given where C++ is re constness, the results might not be that different from option 3)

Personally, for a quick solution I'd go with either 2 or 3.
Your call.

Copy link
Contributor Author

@AniketDalvi AniketDalvi Jul 27, 2020

Choose a reason for hiding this comment

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

Thought about this a little. Given a couple of factors - what this branch is trying to achieve and considering that I have just a couple of weeks left, I think it will be good idea for me to create an issue outlining this very relevant issue you bring up which can be tackled later. The only reason I say that is because I feel this deviates a little from the original purpose of this branch, and that making this const change is going to take some time as it very deeply entrenched in the simulator codebase, and I am hoping to get a couple of more branches checked-in before the end of my internship, and want to leave enough time for that. Thank you!

fusedgates = newFusedGates;
}

template <class T, class A>
void flush(std::vector<T, A>& wfn) const
{
Expand Down Expand Up @@ -79,16 +86,6 @@ class Fused
fusedgates = Fusion();
}

template <class T, class A1, class A2>
bool subsytemwavefunction(std::vector<T, A1>& wfn,
std::vector<unsigned> const& qs,
std::vector<T, A2>& qubitswfn,
double tolerance)
{
flush(wfn); // we have to flush before we can extract the state
return kernels::subsytemwavefunction(wfn, qs, qubitswfn, tolerance);
}

template <class M>
Fusion::Matrix convertMatrix(M const& m)
{
Expand All @@ -102,11 +99,25 @@ class Fused
template <class T, class A, class M>
void apply_controlled(std::vector<T, A>& wfn, M const& mat, std::vector<unsigned> const& cs, unsigned q)
{
// Major runtime logic change here
Fusion::IndexVector qs = std::vector<unsigned>(1, q);
fusedgates.insert(convertMatrix(mat), qs, cs);
}

// Have to update capacity as the WFN grows
template <class T, class A, class M>
void apply(std::vector<T, A>& wfn, M const& mat, unsigned q)
{
std::vector<unsigned> cs;
apply_controlled(wfn, mat, cs, q);
}

template <class T, class A>
bool shouldFlush(std::vector<T, A>& wfn, std::vector<unsigned> const& cs, unsigned q)
{
// Major runtime logic change here

// Have to update capacity as the WFN grows
if (wfnCapacity != wfn.capacity()) {
wfnCapacity = wfn.capacity();
wfnCapacity = wfn.capacity();
char* envNT = NULL;
size_t len;
#ifdef _MSC_VER
Expand All @@ -133,16 +144,9 @@ class Fused
}

// New rules of when to stop fusing
Fusion::IndexVector qs = std::vector<unsigned>(1, q);
if (fusedgates.predict(qs, cs) > maxFusedSpan || fusedgates.size() >= maxFusedDepth) flush(wfn);
fusedgates.insert(convertMatrix(mat), qs, cs);
}
Fusion::IndexVector qs = std::vector<unsigned>(1, q);

template <class T, class A, class M>
void apply(std::vector<T, A>& wfn, M const& mat, unsigned q)
{
std::vector<unsigned> cs;
apply_controlled(wfn, mat, cs, q);
return (fusedgates.predict(qs, cs) > maxFusedSpan || fusedgates.size() >= maxFusedDepth);
}
private:
mutable Fusion fusedgates;
Expand Down
74 changes: 59 additions & 15 deletions src/Simulation/Native/src/external/fusion.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,21 +10,28 @@
#include <iostream>
#include <cassert>
#include "util/alignedalloc.hpp"
#include <unordered_map>

class Item{
public:
using Index = unsigned;
using IndexVector = std::vector<Index>;
using Complex = std::complex<double>;
using Matrix = std::vector<std::vector<Complex, Microsoft::Quantum::Simulator::AlignedAlloc<Complex, 64>>>;
Item(Matrix mat, IndexVector idx) : mat_(mat), idx_(idx) {}
Item(Matrix mat, IndexVector idx) : mat_(std::move(mat)), idx_(idx) {}
Matrix& get_matrix() { return mat_; }
IndexVector& get_indices() { return idx_; }
IndexVector& get_indices() const { return idx_; }
void remap_idx(std::unordered_map<unsigned, unsigned> elemDict) const {
for (size_t i = 0; i < idx_.size(); i++) {
idx_[i] = elemDict[idx_[i]];
}
}
private:
Matrix mat_;
IndexVector idx_;
mutable IndexVector idx_;
};

// Class handling the fusion of gates
class Fusion{
public:
using Index = unsigned;
Expand All @@ -37,7 +44,7 @@ class Fusion{
Fusion() : global_factor_(1.) {}

Index num_qubits() const {
return static_cast<Index>(set_.size());
return static_cast<Index>(target_set_.size());
}

Index num_controls() const {
Expand All @@ -58,21 +65,58 @@ class Fusion{
handle_controls(empty_matrix, empty_vec, {}); // remove all current control qubits (this is a GLOBAL factor)
}

const IndexSet& get_target_set() const {
return target_set_;
}

const ItemVector& get_items() const {
return items_;
}

const IndexSet& get_ctrl_set() const {
return ctrl_set_;
}

const Complex& get_global_factor() const {
return global_factor_;
}

static void remap_qubits(std::set<Index>& qubits, const std::unordered_map<unsigned, unsigned>& mapFromOldLocToNewLoc) {
std::set<Index> tempSet;
for (unsigned elem : qubits) {
if (mapFromOldLocToNewLoc.find(elem) != mapFromOldLocToNewLoc.end()) {
tempSet.insert(mapFromOldLocToNewLoc.at(elem));
}
}
qubits.swap(tempSet);
}

void remap_target_set(const std::unordered_map<unsigned, unsigned>& mapFromOldLocToNewLoc) const {
remap_qubits(target_set_, mapFromOldLocToNewLoc);
}

void remap_ctrl_set(const std::unordered_map<unsigned, unsigned>& mapFromOldLocToNewLoc) const {
remap_qubits(ctrl_set_, mapFromOldLocToNewLoc);
}

void set_items(ItemVector&& newItems) {
items_.swap(newItems);
}

// This saves a class instance create/destroy on every gate insert
// Need a quick way to decide if we're going to grow too wide
int predict(IndexVector index_list, IndexVector const& ctrl_list = {}) {
int cnt = num_qubits() + num_controls();
for (auto idx : index_list)
if (set_.count(idx) == 0 && ctrl_set_.count(idx) == 0) cnt++;
if (target_set_.count(idx) == 0 && ctrl_set_.count(idx) == 0) cnt++;
for (auto idx : ctrl_list)
if (set_.count(idx) == 0 && ctrl_set_.count(idx) == 0) cnt++;
if (target_set_.count(idx) == 0 && ctrl_set_.count(idx) == 0) cnt++;
return cnt;
}

void insert(Matrix matrix, IndexVector index_list, IndexVector const& ctrl_list = {}){
for (auto idx : index_list)
set_.emplace(idx);
target_set_.emplace(idx);

if (global_factor_ != 1. && ctrl_list.size() > 0){
assert(ctrl_set_.size() == 0);
Expand All @@ -85,15 +129,15 @@ class Fusion{
}

void get_indices(IndexVector &indices) const{
for (auto idx : set_)
for (auto idx : target_set_)
indices.push_back(idx);
}

void perform_fusion(Matrix& fused_matrix, IndexVector& index_list, IndexVector& ctrl_list){
if (global_factor_ != 1.)
assert(ctrl_set_.size() == 0);

for (auto idx : set_)
for (auto idx : target_set_)
index_list.push_back(idx);

unsigned N = num_qubits();
Expand Down Expand Up @@ -167,7 +211,7 @@ class Fusion{
if (ctrl_set_.count(ctrlIdx) == 0){ // need to either add it to the list or to the command
if (items_.size() > 0){ // add it to the command
add_controls(matrix, indexList, {ctrlIdx});
set_.insert(ctrlIdx);
target_set_.insert(ctrlIdx);
}
else // add it to the list
ctrl_set_.emplace(ctrlIdx);
Expand All @@ -183,17 +227,17 @@ class Fusion{
for (auto idx : unhandled_ctrl){
new_ctrls.push_back(idx);
ctrl_set_.erase(idx);
set_.insert(idx);
target_set_.insert(idx);
}
for (auto &item : items_)
add_controls(item.get_matrix(), item.get_indices(), new_ctrls);
}
}

IndexSet set_;
ItemVector items_;
IndexSet ctrl_set_;
Complex global_factor_;
mutable IndexSet target_set_; //set of qubits being acted on
Copy link
Contributor

@IrinaYatsenko IrinaYatsenko Jul 29, 2020

Choose a reason for hiding this comment

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

I still believe that it would be better to remove const from flush - it just makes no sense that all members of a class are mutable...

mutable ItemVector items_; //queue if gates to be fused
mutable IndexSet ctrl_set_; //set of controls
mutable Complex global_factor_;
};

#endif
26 changes: 13 additions & 13 deletions src/Simulation/Native/src/external/nointrin/kernel1.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,20 +60,20 @@ void kernel(V& psi, unsigned id0, M const& matrix, std::size_t ctrlmask)
}
}
#else
std::intptr_t zero = 0;
std::intptr_t dmask = dsorted[0];
std::intptr_t zero = 0;
std::intptr_t dmask = dsorted[0];

if (ctrlmask == 0){
#pragma omp parallel for schedule(static)
for (std::intptr_t i = 0; i < static_cast<std::intptr_t>(n); ++i)
if ((i & dmask) == zero)
kernel_core(psi, i, dsorted[0], mm);
} else {
#pragma omp parallel for schedule(static)
for (std::intptr_t i = 0; i < static_cast<std::intptr_t>(n); ++i)
if ((i & ctrlmask) == ctrlmask && (i & dmask) == zero)
kernel_core(psi, i, dsorted[0], mm);
}
if (ctrlmask == 0){
#pragma omp parallel for schedule(static)
for (std::intptr_t i = 0; i < static_cast<std::intptr_t>(n); ++i)
if ((i & dmask) == zero)
kernel_core(psi, i, dsorted[0], mm);
} else {
#pragma omp parallel for schedule(static)
for (std::intptr_t i = 0; i < static_cast<std::intptr_t>(n); ++i)
if ((i & ctrlmask) == ctrlmask && (i & dmask) == zero)
kernel_core(psi, i, dsorted[0], mm);
}
#endif
}

13 changes: 5 additions & 8 deletions src/Simulation/Native/src/simulator/capi_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,32 +107,29 @@ void test_gates()
allocateQubit(sim_id, 0);
allocateQubit(sim_id, 1);

CRx(sim_id, 1.0, 0, 1);
CRx(sim_id, 1.0, 0, 1);

assert(M(sim_id, 1)==false);
assert(M(sim_id, 1) == false);

X(sim_id, 0);
CRx(sim_id, 1.0, 0, 1);
CRx(sim_id, 1.0, 0, 1);

H(sim_id, 1);
CRx(sim_id, -1.0, 0, 1);
H(sim_id, 1);

assert(M(sim_id, 1)==false);
assert(M(sim_id, 1) == false);

X(sim_id, 1);

assert(M(sim_id, 1)==true);

X(sim_id, 1);
assert(M(sim_id, 1) == true);

release(sim_id, 0);
release(sim_id, 1);

destroy(sim_id);
}


void test_allocate()
{
auto sim_id = init();
Expand Down
Loading