RippleCarryAdderD update #358
Conversation
|
|
msoeken
left a comment
msoeken
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
| (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.
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.
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.
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 Sum) (controls, (auxRegister[nQubits-2], xs![nQubits-1], ys![nQubits-1])); | ||
| } | ||
| } | ||
| (Controlled CNOT) (controls,(xs![0], ys![0])); |
There was a problem hiding this comment.
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.
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.
msoeken
left a comment
msoeken
left a comment
There was a problem hiding this comment.
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
msoeken
added
the
Hacktoberfest-Accepted
2 participants
PR for issue #335
Implemented controlled adder blocks following this paper but use the sum and carry blocks to compute the output carry bit.