Breaking: fix frame dependency calculation (#76)

* Fix: frame dependency calculation

* Breaking: Program.get_frames_for_instruction returns HashSet

src/program/frame.rs

@@ -129,7 +129,7 @@ pub(crate) enum FrameMatchCondition<'a> {
     /// Match all frames which contain exactly these qubits
     ExactQubits(&'a [Qubit]),
 
-    /// Return these specific frames, if present in the set
+    /// Return this specific frame, if present in the set
     Specific(&'a FrameIdentifier),
 
     /// Return all frames which match all of these conditions

src/program/graph.rs

@@ -200,6 +200,81 @@ pub struct InstructionBlock {
     pub terminator: BlockTerminator,
 }
 
+/// PreviousNodes is a structure which helps maintain ordering among instructions which operate on a given frame.
+/// It works similarly to a multiple-reader-single-writer queue, where an instruction which "uses" a frame is like
+/// a writer and an instruction which blocks that frame is like a reader. Multiple instructions may concurrently
+/// block a frame, but an instruction may not use a frame while it is concurrently used or blocked.
+///
+/// ## Examples
+///
+/// Note that "depends on" is equivalent to "must execute after completion of".
+///
+/// ```text
+/// user --> user # a second user takes a dependency on the first
+///
+/// user --> blocker # multiple blockers take a dependency on the most recent user
+///      \-> blocker
+///      \-> blocker
+///
+/// blocker --> user --> blocker # users and blockers take dependencies on one another,
+///                              # but blockers do not depend on other blocking instructions
+/// ```
+struct PreviousNodes {
+    using: Option<ScheduledGraphNode>,
+    blocking: HashSet<ScheduledGraphNode>,
+}
+
+impl Default for PreviousNodes {
+    /// The default value for [PreviousNodes] is useful in that, if no previous nodes have been recorded
+    /// as using a frame, we should consider that the start of the instruction block "blocks" use of that frame
+    /// (in other words, this instruction cannot be scheduled prior to the start of the instruction block).
+    fn default() -> Self {
+        Self {
+            using: None,
+            blocking: vec![ScheduledGraphNode::BlockStart].into_iter().collect(),
+        }
+    }
+}
+
+impl PreviousNodes {
+    /// Register a node as using a frame, and return the instructions on which it should depend/wait for scheduling (if any).
+    ///
+    /// A node which uses a frame will block on any previous user or blocker of the frame, much like a writer in a read-write lock.
+    fn get_dependencies_for_next_user(
+        &mut self,
+        node: ScheduledGraphNode,
+    ) -> HashSet<ScheduledGraphNode> {
+        let mut result = std::mem::take(&mut self.blocking);
+        if let Some(previous_user) = self.using.replace(node) {
+            result.insert(previous_user);
+        }
+
+        result
+    }
+
+    /// Register a node as blocking a frame, and return the instructions on which it should depend/wait for scheduling (if any).
+    ///
+    /// A node which blocks a frame will block on any previous user of the frame, but not concurrent blockers.
+    ///
+    /// If the frame is currently blocked by other nodes, it will add itself to the list of blockers,
+    /// much like a reader in a read-write lock.
+    fn get_dependency_for_next_blocker(
+        &mut self,
+        node: ScheduledGraphNode,
+    ) -> Option<ScheduledGraphNode> {
+        self.blocking.insert(node);
+        self.using
+    }
+
+    /// Consume the [PreviousNodes] and return all nodes within.
+    pub fn drain(mut self) -> HashSet<ScheduledGraphNode> {
+        if let Some(using) = self.using {
+            self.blocking.insert(using);
+        }
+        self.blocking
+    }
+}
+
 impl InstructionBlock {
     pub fn build(
         instructions: Vec<Instruction>,
@@ -213,8 +288,7 @@ impl InstructionBlock {
         let mut last_classical_instruction = ScheduledGraphNode::BlockStart;
 
         // Store the instruction index of the last instruction to block that frame
-        let mut last_instruction_by_frame: HashMap<FrameIdentifier, ScheduledGraphNode> =
-            HashMap::new();
+        let mut last_instruction_by_frame: HashMap<FrameIdentifier, PreviousNodes> = HashMap::new();
 
         // Store memory access reads and writes. Key is memory region name.
         // NOTE: this may be refined to serialize by memory region offset rather than by entire region.
@@ -236,21 +310,32 @@ impl InstructionBlock {
                     let used_frames = program
                         .get_frames_for_instruction(instruction, false)
                         .unwrap_or_default();
+
                     let blocked_frames = program
                         .get_frames_for_instruction(instruction, true)
                         .unwrap_or_default();
 
-                    // Take a dependency on any previous instructions to _block_ a frame which this instruction _uses_.
-                    for frame in used_frames {
-                        let previous_node_id = last_instruction_by_frame
-                            .get(frame)
-                            .unwrap_or(&ScheduledGraphNode::BlockStart);
-                        add_dependency!(graph, *previous_node_id => node, ExecutionDependency::ReferenceFrame);
+                    let blocked_but_not_used_frames = blocked_frames.difference(&used_frames);
+
+                    for frame in &used_frames {
+                        let previous_node_ids = last_instruction_by_frame
+                            .entry((*frame).clone())
+                            .or_default()
+                            .get_dependencies_for_next_user(node);
+
+                        for previous_node_id in previous_node_ids {
+                            add_dependency!(graph, previous_node_id => node, ExecutionDependency::ReferenceFrame);
+                        }
                     }
 
-                    // We mark all "blocked" frames as such for later instructions to take a dependency on
-                    for frame in blocked_frames {
-                        last_instruction_by_frame.insert(frame.clone(), node);
+                    for frame in blocked_but_not_used_frames {
+                        if let Some(previous_node_id) = last_instruction_by_frame
+                            .entry((*frame).clone())
+                            .or_default()
+                            .get_dependency_for_next_blocker(node)
+                        {
+                            add_dependency!(graph, previous_node_id => node, ExecutionDependency::ReferenceFrame);
+                        }
                     }
 
                     Ok(())
@@ -295,8 +380,10 @@ impl InstructionBlock {
         // does not terminate until these are complete
         add_dependency!(graph, last_classical_instruction => ScheduledGraphNode::BlockEnd, ExecutionDependency::StableOrdering);
 
-        for (_, last_instruction) in last_instruction_by_frame {
-            add_dependency!(graph, last_instruction => ScheduledGraphNode::BlockEnd, ExecutionDependency::ReferenceFrame);
+        for previous_nodes in last_instruction_by_frame.into_values() {
+            for node in previous_nodes.drain() {
+                add_dependency!(graph, node => ScheduledGraphNode::BlockEnd, ExecutionDependency::ReferenceFrame);
+            }
         }
 
         // Examine all "pending" memory operations for all regions

src/program/graphviz_dot.rs

@@ -300,6 +300,35 @@ NONBLOCKING PULSE 2 \"rf\" test(duration: 1e6)
 "
         );
 
+        build_dot_format_snapshot_test_case!(
+            blocking_pulses_wrap_nonblocking,
+            "
+PULSE 0 \"rf\" test(duration: 1e6)
+NONBLOCKING PULSE 0 \"ro_tx\" test(duration: 1e6)
+PULSE 0 \"rf\" test(duration: 1e6)
+FENCE 0
+FENCE 0
+"
+        );
+
+        build_dot_format_snapshot_test_case!(
+            blocking_pulses_after_nonblocking,
+            "
+NONBLOCKING PULSE 0 \"ro_tx\" test(duration: 1e6)
+PULSE 0 \"rf\" test(duration: 1e6)
+PULSE 0 \"ro_rx\" test(duration: 1e6)
+"
+        );
+
+        build_dot_format_snapshot_test_case!(
+            blocking_2q_pulse,
+            "
+PULSE 0 \"rf\" test(duration: 1e-6)
+PULSE 1 \"rf\" test(duration: 1e-6)
+PULSE 0 1 \"cz\" test(duration: 1e-6)
+"
+        );
+
         build_dot_format_snapshot_test_case!(
             fence_all_with_nonblocking_pulses,
             "

src/program/mod.rs

@@ -130,15 +130,10 @@ impl Program {
         &'a self,
         instruction: &'a Instruction,
         include_blocked: bool,
-    ) -> Option<Vec<&'a FrameIdentifier>> {
+    ) -> Option<HashSet<&'a FrameIdentifier>> {
         instruction
             .get_frame_match_condition(include_blocked)
-            .map(|condition| {
-                self.frames
-                    .get_matching_keys(condition)
-                    .into_iter()
-                    .collect()
-            })
+            .map(|condition| self.frames.get_matching_keys(condition))
     }
 
     /// Returns a HashSet consisting of every Qubit that is used in the program.

src/program/snapshots/quil_rs__program__graphviz_dot__tests__graph__active_reset_single_frame.snap

@@ -26,7 +26,8 @@ frame"];
     node [style="filled"];
     "feedback_start" [shape=circle, label="start"];
     "feedback_start" -> "feedback_0" [label="frame"];
-    "feedback_start" -> "feedback_end" [label="ordering"];
+    "feedback_start" -> "feedback_end" [label="frame
+ordering"];
     "feedback_0" [shape=rectangle, label="[0] PULSE 0 \"rf\" test(duration: 1000000.0)"];
     "feedback_0" -> "feedback_end" [label="frame"];
     "feedback_end" [shape=circle, label="end"];

src/program/snapshots/quil_rs__program__graphviz_dot__tests__graph__blocking_2q_pulse.snap

@@ -0,0 +1,28 @@
+---
+source: src/program/graphviz_dot.rs
+expression: dot_format
+---
+digraph {
+  entry -> "block_0_start";
+  entry [label="Entry Point"];
+  subgraph cluster_0 {
+    label="block_0";
+    node [style="filled"];
+    "block_0_start" [shape=circle, label="start"];
+    "block_0_start" -> "block_0_0" [label="frame"];
+    "block_0_start" -> "block_0_1" [label="frame"];
+    "block_0_start" -> "block_0_2" [label="frame"];
+    "block_0_start" -> "block_0_end" [label="frame
+ordering"];
+    "block_0_0" [shape=rectangle, label="[0] PULSE 0 \"rf\" test(duration: 1e-6)"];
+    "block_0_0" -> "block_0_2" [label="frame"];
+    "block_0_0" -> "block_0_end" [label="frame"];
+    "block_0_1" [shape=rectangle, label="[1] PULSE 1 \"rf\" test(duration: 1e-6)"];
+    "block_0_1" -> "block_0_2" [label="frame"];
+    "block_0_1" -> "block_0_end" [label="frame"];
+    "block_0_2" [shape=rectangle, label="[2] PULSE 0 1 \"cz\" test(duration: 1e-6)"];
+    "block_0_2" -> "block_0_end" [label="frame"];
+    "block_0_end" [shape=circle, label="end"];
+  }
+}
+

src/program/snapshots/quil_rs__program__graphviz_dot__tests__graph__blocking_pulses_after_nonblocking.snap

@@ -0,0 +1,29 @@
+---
+source: src/program/graphviz_dot.rs
+expression: dot_format
+---
+digraph {
+  entry -> "block_0_start";
+  entry [label="Entry Point"];
+  subgraph cluster_0 {
+    label="block_0";
+    node [style="filled"];
+    "block_0_start" [shape=circle, label="start"];
+    "block_0_start" -> "block_0_0" [label="frame"];
+    "block_0_start" -> "block_0_1" [label="frame"];
+    "block_0_start" -> "block_0_2" [label="frame"];
+    "block_0_start" -> "block_0_end" [label="frame
+ordering"];
+    "block_0_0" [shape=rectangle, label="[0] NONBLOCKING PULSE 0 \"ro_tx\" test(duration: 1000000.0)"];
+    "block_0_0" -> "block_0_1" [label="frame"];
+    "block_0_0" -> "block_0_2" [label="frame"];
+    "block_0_0" -> "block_0_end" [label="frame"];
+    "block_0_1" [shape=rectangle, label="[1] PULSE 0 \"rf\" test(duration: 1000000.0)"];
+    "block_0_1" -> "block_0_2" [label="frame"];
+    "block_0_1" -> "block_0_end" [label="frame"];
+    "block_0_2" [shape=rectangle, label="[2] PULSE 0 \"ro_rx\" test(duration: 1000000.0)"];
+    "block_0_2" -> "block_0_end" [label="frame"];
+    "block_0_end" [shape=circle, label="end"];
+  }
+}
+

src/program/snapshots/quil_rs__program__graphviz_dot__tests__graph__blocking_pulses_wrap_nonblocking.snap

@@ -0,0 +1,32 @@
+---
+source: src/program/graphviz_dot.rs
+expression: dot_format
+---
+digraph {
+  entry -> "block_0_start";
+  entry [label="Entry Point"];
+  subgraph cluster_0 {
+    label="block_0";
+    node [style="filled"];
+    "block_0_start" [shape=circle, label="start"];
+    "block_0_start" -> "block_0_0" [label="frame"];
+    "block_0_start" -> "block_0_1" [label="frame"];
+    "block_0_start" -> "block_0_3" [label="frame"];
+    "block_0_start" -> "block_0_end" [label="ordering"];
+    "block_0_0" [shape=rectangle, label="[0] PULSE 0 \"rf\" test(duration: 1000000.0)"];
+    "block_0_0" -> "block_0_1" [label="frame"];
+    "block_0_0" -> "block_0_2" [label="frame"];
+    "block_0_0" -> "block_0_3" [label="frame"];
+    "block_0_1" [shape=rectangle, label="[1] NONBLOCKING PULSE 0 \"ro_tx\" test(duration: 1000000.0)"];
+    "block_0_1" -> "block_0_2" [label="frame"];
+    "block_0_1" -> "block_0_3" [label="frame"];
+    "block_0_2" [shape=rectangle, label="[2] PULSE 0 \"rf\" test(duration: 1000000.0)"];
+    "block_0_2" -> "block_0_3" [label="frame"];
+    "block_0_3" [shape=rectangle, label="[3] FENCE 0"];
+    "block_0_3" -> "block_0_4" [label="frame"];
+    "block_0_4" [shape=rectangle, label="[4] FENCE 0"];
+    "block_0_4" -> "block_0_end" [label="frame"];
+    "block_0_end" [shape=circle, label="end"];
+  }
+}
+

src/program/snapshots/quil_rs__program__graphviz_dot__tests__graph__chained_pulses.snap

@@ -10,15 +10,20 @@ digraph {
     node [style="filled"];
     "block_0_start" [shape=circle, label="start"];
     "block_0_start" -> "block_0_0" [label="frame"];
-    "block_0_start" -> "block_0_end" [label="ordering"];
+    "block_0_start" -> "block_0_end" [label="frame
+ordering"];
     "block_0_0" [shape=rectangle, label="[0] PULSE 0 \"rf\" test(duration: 1000000.0)"];
     "block_0_0" -> "block_0_1" [label="frame"];
+    "block_0_0" -> "block_0_end" [label="frame"];
     "block_0_1" [shape=rectangle, label="[1] PULSE 0 \"rf\" test(duration: 1000000.0)"];
     "block_0_1" -> "block_0_2" [label="frame"];
+    "block_0_1" -> "block_0_end" [label="frame"];
     "block_0_2" [shape=rectangle, label="[2] PULSE 0 \"rf\" test(duration: 1000000.0)"];
     "block_0_2" -> "block_0_3" [label="frame"];
+    "block_0_2" -> "block_0_end" [label="frame"];
     "block_0_3" [shape=rectangle, label="[3] PULSE 0 \"rf\" test(duration: 1000000.0)"];
     "block_0_3" -> "block_0_4" [label="frame"];
+    "block_0_3" -> "block_0_end" [label="frame"];
     "block_0_4" [shape=rectangle, label="[4] PULSE 0 \"rf\" test(duration: 1000000.0)"];
     "block_0_4" -> "block_0_end" [label="frame"];
     "block_0_end" [shape=circle, label="end"];

src/program/snapshots/quil_rs__program__graphviz_dot__tests__graph__different_frames_blocking.snap

@@ -12,7 +12,8 @@ digraph {
     "block_0_start" -> "block_0_0" [label="frame"];
     "block_0_start" -> "block_0_1" [label="frame"];
     "block_0_start" -> "block_0_2" [label="frame"];
-    "block_0_start" -> "block_0_end" [label="ordering"];
+    "block_0_start" -> "block_0_end" [label="frame
+ordering"];
     "block_0_0" [shape=rectangle, label="[0] PULSE 0 \"rf\" test(duration: 1000000.0)"];
     "block_0_0" -> "block_0_end" [label="frame"];
     "block_0_1" [shape=rectangle, label="[1] PULSE 1 \"rf\" test(duration: 1000000.0)"];

src/program/snapshots/quil_rs__program__graphviz_dot__tests__graph__jump.snap

@@ -10,7 +10,8 @@ digraph {
     node [style="filled"];
     "first-block_start" [shape=circle, label="start"];
     "first-block_start" -> "first-block_0" [label="frame"];
-    "first-block_start" -> "first-block_end" [label="ordering"];
+    "first-block_start" -> "first-block_end" [label="frame
+ordering"];
     "first-block_0" [shape=rectangle, label="[0] PULSE 0 \"rf\" test(duration: 1000000.0)"];
     "first-block_0" -> "first-block_end" [label="frame"];
     "first-block_end" [shape=circle, label="end"];
@@ -22,7 +23,8 @@ digraph {
     node [style="filled"];
     "second-block_start" [shape=circle, label="start"];
     "second-block_start" -> "second-block_0" [label="frame"];
-    "second-block_start" -> "second-block_end" [label="ordering"];
+    "second-block_start" -> "second-block_end" [label="frame
+ordering"];
     "second-block_0" [shape=rectangle, label="[0] PULSE 0 \"rf\" test(duration: 1000000.0)"];
     "second-block_0" -> "second-block_end" [label="frame"];
     "second-block_end" [shape=circle, label="end"];
@@ -33,7 +35,8 @@ digraph {
     node [style="filled"];
     "third-block_start" [shape=circle, label="start"];
     "third-block_start" -> "third-block_0" [label="frame"];
-    "third-block_start" -> "third-block_end" [label="ordering"];
+    "third-block_start" -> "third-block_end" [label="frame
+ordering"];
     "third-block_0" [shape=rectangle, label="[0] PULSE 0 \"rf\" test(duration: 1000000.0)"];
     "third-block_0" -> "third-block_end" [label="frame"];
     "third-block_end" [shape=circle, label="end"];

src/program/snapshots/quil_rs__program__graphviz_dot__tests__graph__parametric_pulse.snap

@@ -10,10 +10,13 @@ digraph {
     node [style="filled"];
     "block_0_start" [shape=circle, label="start"];
     "block_0_start" -> "block_0_0" [label="frame"];
-    "block_0_start" -> "block_0_end" [label="ordering"];
+    "block_0_start" -> "block_0_1" [label="frame"];
+    "block_0_start" -> "block_0_end" [label="frame
+ordering"];
     "block_0_0" [shape=rectangle, label="[0] PULSE 0 \"rf\" test(a: param[0])"];
     "block_0_0" -> "block_0_1" [label="frame"];
-    "block_0_0" -> "block_0_end" [label="await read"];
+    "block_0_0" -> "block_0_end" [label="await read
+frame"];
     "block_0_1" [shape=rectangle, label="[1] CAPTURE 0 \"ro_rx\" test(a: param[0]) ro[0]"];
     "block_0_1" -> "block_0_end" [label="await capture
 await read