-
Notifications
You must be signed in to change notification settings - Fork 177
Conversation
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.
Thanks a lot for your pull request @TviNet. It looks already very good. I left some comments.
Standard/src/Arithmetic/Integer.qs
Outdated
/// `summand1` and `summand2`. | ||
/// ## carryOut | ||
/// Carry-out qubit, will be xored with the higher bit of the sum. | ||
operation AdderBlockUsingTemporaryLogicalAnd(carryIn: Qubit, summand1: Qubit, summand2: Qubit, carryOut: Qubit) |
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 operation can be declared internal
as it is only used for the implementation of RippleCarryAdderD
.
Standard/src/Arithmetic/Integer.qs
Outdated
/// ## carryOut | ||
/// Carry-out qubit, will be xored with the higher bit of the sum. | ||
operation AdderBlockUsingTemporaryLogicalAnd(carryIn: Qubit, summand1: Qubit, summand2: Qubit, carryOut: Qubit) | ||
: Unit is Adj + Ctl { |
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 operation does not need to be Ctl
, since it is only called inside a within
block. Then the controlled blocks can be removed and also the body (...)
line can be removed, as it will be the only specialisation (the adjoint
can be generated automatically).
Standard/src/Arithmetic/Integer.qs
Outdated
Carry(auxRegister[idx], xs![idx], ys![idx], auxRegister[idx+1]); // (1) | ||
using (auxRegister = Qubit[nQubits-1]) { | ||
within { | ||
ApplyAnd (xs![0], ys![0], auxRegister[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.
It should be possible to merge these nested within / apply
blocks by moving this ApplyAnd
operation call before the for
loop in the nested within
block.
Standard/src/Arithmetic/Integer.qs
Outdated
apply { | ||
within { | ||
for (idx in 1..(nQubits-2)) { | ||
(Controlled AdderBlockUsingTemporaryLogicalAnd) (controls, (auxRegister[idx-1], xs![idx], ys![idx], auxRegister[idx])); |
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.
(Controlled AdderBlockUsingTemporaryLogicalAnd) (controls, (auxRegister[idx-1], xs![idx], ys![idx], auxRegister[idx])); | |
AdderBlockUsingTemporaryLogicalAnd(auxRegister[idx-1], xs![idx], ys![idx], auxRegister[idx]); |
This statement does not need to be controlled, as it is inside a within
block.
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.
Thanks for the suggestions
I am using this figure as reference.
The block can be considered to have two parts: ComputeTempCarry
and EraseTempCarry
.
To make use of within..apply structure I have used
AdderBlockUsingTemporaryLogicalAnd = ComputeTempCarry
Adjoint AdderBlockUsingTemporaryLogicalAnd = EraseTempCarry
Since the control only affects EraseTempCarry
I had to add a Controlled + Adjoint variant.
I realize this has gotten a bit messy and is probably an incorrect usage of adjoint
So I am planning to use the two blocks ComputeTempCarry is Adj
and EraseTempCarry is Adj + Ctl
but this would not make use of within..apply
Standard/src/Arithmetic/Integer.qs
Outdated
} | ||
} | ||
apply { | ||
(Controlled Carry) (controls, (auxRegister[nQubits-2], xs![nQubits-1], ys![nQubits-1], carry)); |
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 Carry
implementation uses two CCNOT
gates and could be replaced by something that uses a single ApplyAnd
. Could AdderBlockUsingTemporaryLogicalAnd
be used?
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.
AdderBlockUsingTemporaryLogicalAnd
has to erase the carry bit by design so I don't think it can be used here unless I understand incorrectly. The first CCNOT
of Carry
can be changed to ApplyAnd
since the CarryOut
is zero. I haven't been able to find another implementation that would do better.
Standard/src/Arithmetic/Integer.qs
Outdated
} | ||
(Controlled CNOT) (controls,(xs![0], ys![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.
It is not clear to me why this line is here. Could you add some line of documentation to make it clearer.
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.
Since the first bit has CarryIn
always zero the operations on first bit can be simplified to
ApplyAnd (xs![0], ys![0], auxRegister[0]);
---<Compute other bits using auxRegister[0]>---
(Adjoint ApplyAnd) (xs![0], ys![0], auxRegister[0]);
(Controlled CNOT) (controls,(xs![0], ys![0]));
to reduce the gate count.
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.
Thanks for your changes and the clarification. I approve this PR and changed the target to a feature branch.
Thanks for your work on this @TviNet
PR for issue #335
Implemented controlled adder blocks following this paper but use the sum and carry blocks to compute the output carry bit.