Fix data-flow analysis in control-flow jump/mark (Qiskit#1666)

* Fix data-flow analysis in control-flow jump/mark

During the inlining of control-flow into jump/mark instructions, the
data-flow graph was losing information.  The tree-like form within
`QuantumCircuit` implicitly creates a basic-block structure, which
groups the whole control-flow body into a single block that must be
executed atomicly (as seen by the outer circuit).  When inlining the
body, this is no longer the case, and we have to take more care to
ensure that topological iteration through the data-flow graph does not
accidentally move jump/mark instructions into incorrect places, nor
allow unrelated instructions to appear between jump/mark pairs if they
were no originally in that control-flow block.

There are two components here; the former is solved by ensuring the
jump/mark instructions span the full data width including clbits, and
the latter is solved by placing full circuit width barriers around each
control-flow logical component.

* Fix lint

qiskit_aer/backends/aer_compiler.py

@@ -12,7 +12,10 @@
 """
 Compier to convert Qiskit control-flow to Aer backend.
 """
-from qiskit.circuit import QuantumCircuit
+
+import itertools
+
+from qiskit.circuit import QuantumCircuit, Clbit
 from qiskit.extensions import Initialize
 from qiskit.pulse import Schedule, ScheduleBlock
 from qiskit.circuit.controlflow import (
@@ -117,61 +120,76 @@ def _is_dynamic(circuit, optype=None):
             return bool(optype.intersection(controlflow_types))
 
         # Check via iteration
-        for inst, _, _ in circuit.data:
-            if isinstance(inst, controlflow_types):
+        for instruction in circuit.data:
+            if isinstance(instruction.operation, controlflow_types):
                 return True
 
         return False
 
-    def _inline_circuit(self, circ, continue_label, break_label):
+    def _inline_circuit(self, circ, continue_label, break_label, bit_map=None):
         """convert control-flow instructions to mark and jump instructions
 
         Args:
             circ (QuantumCircuit): The QuantumCircuit to be compiled
             continue_label (str): label name for continue.
             break_label (str): label name for break.
+            bit_map (dict[Bit, Bit]): mapping of virtual bits in the current circuit to the bit they
+                represent in the outermost circuit.
 
         Returns:
             QuantumCircuit: QuantumCircuit without control-flow instructions
         """
-        ret = circ.copy()
-        ret.data = []
-
-        q2i = {}
-        for q in ret.qubits:
-            q2i[q] = len(q2i)
-        c2i = {}
-        for c in ret.clbits:
-            c2i[c] = len(c2i)
-
-        for inst, qargs, cargs in circ.data:
-            binding_qargs = [q2i[q] for q in qargs]
-            binding_cargs = [c2i[c] for c in cargs]
-            if isinstance(inst, ForLoopOp):
-                self._inline_for_loop_op(inst, ret, binding_qargs, binding_cargs)
-            elif isinstance(inst, WhileLoopOp):
-                self._inline_while_loop_op(inst, ret, binding_qargs, binding_cargs)
-            elif isinstance(inst, IfElseOp):
-                self._inline_if_else_op(inst, continue_label, break_label,
-                                        ret, binding_qargs, binding_cargs)
-            elif isinstance(inst, BreakLoopOp):
-                ret.append(AerJump(break_label, ret.num_qubits),
-                           range(ret.num_qubits), [])
-            elif isinstance(inst, ContinueLoopOp):
-                ret.append(AerJump(continue_label, ret.num_qubits),
-                           range(ret.num_qubits), [])
+        ret = circ.copy_empty_like()
+        bit_map = {bit: bit for bit in itertools.chain(ret.qubits, ret.clbits)}
+
+        for instruction in circ.data:
+            # The barriers around all control-flow operations is to prevent any non-control-flow
+            # operations from ending up topologically "inside" a body.  This can happen if the body
+            # is not full width on the circuit, and the other operation uses disjoint bits.
+            if isinstance(instruction.operation, ForLoopOp):
+                ret.barrier()
+                self._inline_for_loop_op(instruction, ret, bit_map)
+                ret.barrier()
+            elif isinstance(instruction.operation, WhileLoopOp):
+                ret.barrier()
+                self._inline_while_loop_op(instruction, ret, bit_map)
+                ret.barrier()
+            elif isinstance(instruction.operation, IfElseOp):
+                ret.barrier()
+                self._inline_if_else_op(instruction, continue_label, break_label, ret, bit_map)
+                ret.barrier()
+            elif isinstance(instruction.operation, BreakLoopOp):
+                ret._append(
+                    AerJump(break_label, ret.num_qubits, ret.num_clbits), ret.qubits, ret.clbits
+                )
+            elif isinstance(instruction.operation, ContinueLoopOp):
+                ret._append(
+                    AerJump(continue_label, ret.num_qubits, ret.num_clbits), ret.qubits, ret.clbits
+                )
             else:
-                ret.append(inst, qargs, cargs)
+                ret._append(instruction)
 
         return ret
 
-    def _convert_c_if_args(self, cond_tuple):
-        """convert a boolean value to 0 or 1 in c_if elements"""
-        return [1 if elem is True else 0 if elem is False else elem for elem in cond_tuple]
+    def _convert_c_if_args(self, cond_tuple, bit_map):
+        """Convert a condition tuple according to the wire map."""
+        if isinstance(cond_tuple[0], Clbit):
+            return (bit_map[cond_tuple[0]], cond_tuple[1])
+        # ClassicalRegister conditions should already be in the outer circuit.
+        return cond_tuple
 
-    def _inline_for_loop_op(self, inst, parent, qargs, cargs):
+    def _inline_for_loop_op(self, instruction, parent, bit_map):
         """inline for_loop body while iterating its indexset"""
-        indexset, loop_parameter, body = inst.params
+        qargs = [bit_map[q] for q in instruction.qubits]
+        cargs = [bit_map[c] for c in instruction.clbits]
+        indexset, loop_parameter, body = instruction.operation.params
+        inner_bit_map = {
+            inner: bit_map[outer]
+            for inner, outer in itertools.chain(
+                zip(body.qubits, instruction.qubits),
+                zip(body.clbits, instruction.clbits),
+            )
+        }
 
         self._last_flow_id += 1
         loop_id = self._last_flow_id
@@ -181,21 +199,19 @@ def _inline_for_loop_op(self, inst, parent, qargs, cargs):
         break_label = f'{loop_name}_end'
         for index in indexset:
             continue_label = f'{loop_name}_{index}'
-            inlined_body = self._inline_circuit(body,
-                                                continue_label,
-                                                break_label)
+            inlined_body = self._inline_circuit(body, continue_label, break_label, inner_bit_map)
             if loop_parameter is not None:
                 inlined_body = inlined_body.bind_parameters({loop_parameter: index})
             parent.append(inlined_body, qargs, cargs)
-            parent.append(AerMark(continue_label, inlined_body.num_qubits), qargs, [])
+            parent.append(AerMark(continue_label, len(qargs), len(cargs)), qargs, cargs)
 
-        if inlined_body:
-            parent.append(AerMark(break_label, inlined_body.num_qubits), qargs, [])
+        if inlined_body is not None:
+            parent.append(AerMark(break_label, len(qargs), len(cargs)), qargs, cargs)
 
-    def _inline_while_loop_op(self, inst, parent, qargs, cargs):
+    def _inline_while_loop_op(self, instruction, parent, bit_map):
         """inline while_loop body with jump and mark instructions"""
-        condition_tuple = inst.condition
-        body, = inst.params
+        condition_tuple = self._convert_c_if_args(instruction.operation.condition, bit_map)
+        body, = instruction.operation.params
 
         self._last_flow_id += 1
         loop_id = self._last_flow_id
@@ -204,23 +220,44 @@ def _inline_while_loop_op(self, inst, parent, qargs, cargs):
         continue_label = f'{loop_name}_continue'
         loop_start_label = f'{loop_name}_start'
         break_label = f'{loop_name}_end'
-        inlined_body = self._inline_circuit(body, continue_label, break_label)
-
-        c_if_args = self._convert_c_if_args(condition_tuple)
+        inlined_body = self._inline_circuit(
+            body,
+            continue_label,
+            break_label,
+            {
+                inner: bit_map[outer]
+                for inner, outer in itertools.chain(
+                    zip(body.qubits, instruction.qubits),
+                    zip(body.clbits, instruction.clbits),
+                )
+            },
+        )
+        qargs = [bit_map[q] for q in instruction.qubits]
+        cargs = [bit_map[c] for c in instruction.clbits]
+        mark_cargs = cargs.copy()
+        mark_cargs.extend(
+            bit_map[c] for c in (
+                (
+                    {condition_tuple[0]} if isinstance(condition_tuple[0], Clbit)
+                    else set(condition_tuple[0])
+                ) - set(instruction.clbits)
+            )
+        )
+        c_if_args = self._convert_c_if_args(condition_tuple, bit_map)
 
-        parent.append(AerMark(continue_label, inlined_body.num_qubits), qargs, [])
-        parent.append(AerJump(loop_start_label, inlined_body.num_qubits).c_if(*c_if_args),
-                      qargs, [])
-        parent.append(AerJump(break_label, inlined_body.num_qubits), qargs, [])
-        parent.append(AerMark(loop_start_label, inlined_body.num_qubits), qargs, [])
+        parent.append(AerMark(continue_label, len(qargs), len(mark_cargs)), qargs, mark_cargs)
+        parent.append(AerJump(loop_start_label, len(qargs), len(mark_cargs)).c_if(*c_if_args),
+                      qargs, mark_cargs)
+        parent.append(AerJump(break_label, len(qargs), len(mark_cargs)), qargs, mark_cargs)
+        parent.append(AerMark(loop_start_label, len(qargs), len(mark_cargs)), qargs, mark_cargs)
         parent.append(inlined_body, qargs, cargs)
-        parent.append(AerJump(continue_label, inlined_body.num_qubits), qargs, [])
-        parent.append(AerMark(break_label, inlined_body.num_qubits), qargs, [])
+        parent.append(AerJump(continue_label, len(qargs), len(mark_cargs)), qargs, mark_cargs)
+        parent.append(AerMark(break_label, len(qargs), len(mark_cargs)), qargs, mark_cargs)
 
-    def _inline_if_else_op(self, inst, continue_label, break_label, parent, qargs, cargs):
+    def _inline_if_else_op(self, instruction, continue_label, break_label, parent, bit_map):
         """inline true and false bodies of if_else with jump and mark instructions"""
-        condition_tuple = inst.condition
-        true_body, false_body = inst.params
+        condition_tuple = instruction.operation.condition
+        true_body, false_body = instruction.operation.params
 
         self._last_flow_id += 1
         if_id = self._last_flow_id
@@ -233,20 +270,54 @@ def _inline_if_else_op(self, inst, continue_label, break_label, parent, qargs, c
         else:
             if_else_label = if_end_label
 
-        c_if_args = self._convert_c_if_args(condition_tuple)
+        c_if_args = self._convert_c_if_args(condition_tuple, bit_map)
+
+        qargs = [bit_map[q] for q in instruction.qubits]
+        cargs = [bit_map[c] for c in instruction.clbits]
+        mark_cargs = cargs.copy()
+        mark_cargs.extend(
+            bit_map[c] for c in (
+                (
+                    {condition_tuple[0]} if isinstance(condition_tuple[0], Clbit)
+                    else set(condition_tuple[0])
+                ) - set(instruction.clbits)
+            )
+        )
 
-        parent.append(AerJump(if_true_label, true_body.num_qubits).c_if(*c_if_args), qargs, [])
-        parent.append(AerJump(if_else_label, true_body.num_qubits), qargs, [])
-        parent.append(AerMark(if_true_label, true_body.num_qubits), qargs, [])
-        parent.append(self._inline_circuit(true_body, continue_label, break_label), qargs, cargs)
+        true_bit_map = {
+            inner: bit_map[outer]
+            for inner, outer in itertools.chain(
+                zip(true_body.qubits, instruction.qubits),
+                zip(true_body.clbits, instruction.clbits),
+            )
+        }
 
-        if false_body:
-            parent.append(AerJump(if_end_label, true_body.num_qubits), qargs, [])
-            parent.append(AerMark(if_else_label, true_body.num_qubits), qargs, [])
-            parent.append(self._inline_circuit(false_body, continue_label, break_label),
-                          qargs, cargs)
+        parent.append(
+            AerJump(if_true_label, len(qargs), len(mark_cargs)).c_if(*c_if_args), qargs, mark_cargs
+        )
+        parent.append(AerJump(if_else_label, len(qargs), len(mark_cargs)), qargs, mark_cargs)
+        parent.append(AerMark(if_true_label, len(qargs), len(mark_cargs)), qargs, mark_cargs)
+        parent.append(
+            self._inline_circuit(true_body, continue_label, break_label, true_bit_map), qargs, cargs
+        )
 
-        parent.append(AerMark(if_end_label, true_body.num_qubits), qargs, [])
+        if false_body:
+            false_bit_map = {
+                inner: bit_map[outer]
+                for inner, outer in itertools.chain(
+                    zip(false_body.qubits, instruction.qubits),
+                    zip(false_body.clbits, instruction.clbits),
+                )
+            }
+            parent.append(AerJump(if_end_label, len(qargs), len(mark_cargs)), qargs, mark_cargs)
+            parent.append(AerMark(if_else_label, len(qargs), len(mark_cargs)), qargs, mark_cargs)
+            parent.append(
+                self._inline_circuit(false_body, continue_label, break_label, false_bit_map),
+                qargs,
+                cargs,
+            )
+
+        parent.append(AerMark(if_end_label, len(qargs), len(mark_cargs)), qargs, mark_cargs)
 
 
 def compile_circuit(circuits, basis_gates=None, optypes=None):

qiskit_aer/library/control_flow_instructions/jump.py

@@ -25,5 +25,5 @@ class AerJump(Instruction):
 
     _directive = True
 
-    def __init__(self, jump_to, num_qubits):
-        super().__init__("jump", num_qubits, 0, [jump_to])
+    def __init__(self, jump_to, num_qubits, num_clbits=0):
+        super().__init__("jump", num_qubits, num_clbits, [jump_to])

qiskit_aer/library/control_flow_instructions/mark.py

@@ -26,5 +26,5 @@ class AerMark(Instruction):
 
     _directive = True
 
-    def __init__(self, name, num_qubits):
-        super().__init__("mark", num_qubits, 0, [name])
+    def __init__(self, name, num_qubits, num_clbits=0):
+        super().__init__("mark", num_qubits, num_clbits, [name])

releasenotes/notes/fix-topological-control-flow-e2f1a25098004f00.yaml

@@ -0,0 +1,29 @@
+---
+fixes:
+  - |
+    Fixed incorrect logic in the control-flow compiler that could allow unrelated instructions to
+    appear "inside" control-flow bodies during execution, causing incorrect results.  For example,
+    previously::
+
+        from qiskit import QuantumCircuit
+        from qiskit_aer import AerSimulator
+
+        backend = AerSimulator(method="statevector")
+
+        circuit = QuantumCircuit(3, 3)
+        circuit.measure(0, 0)
+        circuit.measure(1, 1)
+
+        with circuit.if_test((0, True)):
+            with circuit.if_test((1, False)):
+                circuit.x(2)
+
+        with circuit.if_test((0, False)):
+            with circuit.if_test((1, True)):
+                circuit.x(2)
+
+        circuit.measure(range(3), range(3))
+        print(backend.run(circuit, method=method, shots=100).result())
+
+    would print ``{'010': 100}`` as the nested control-flow operations would accidentally jump over
+    the first X gate on qubit 2, which should have been executed.