Extend ProgramStats PR #339

kalzoo · 2024-02-09T07:20:11Z

Extends #336 with:

refactoring to better organize files under the program module. Note that all the shiny new stuff is now at program::analysis. I'd like to re-export structs from that level rather than make submodules private, for user benefit. A user shouldn't have to burrow more than 2 levels deep here, anything deeper is an implementation detail.
ControlFlowGraph and BasicBlock which are neatly constructed from (and which borrow) a Program
- then, a refactor of ScheduledProgram with InstructionBlocks to ScheduledControlFlowGraph with ScheduledBasicBlocks
- This change is obvious in hindsight. The ScheduledProgram was overloaded with two functions: control flow and Quil pulse program vector clock construction. These are distinct. Combining them into one pass means that programs which contain gates and other non-pulse-level instructions but which benefit from control flow analysis
- Differences from the previous InstructionBlock implementation:
  - Unlabeled blocks now don't have labels auto-generated but rather have a label of None. Blocks are stored in a Vec rather than an index map. Blocks which do not end with a terminator now Continue into the block of succeeding index rather than Jumping there. This is more intuitive behavior.
  - ControlFlowGraph construction is much simpler and easier to follow logically than ScheduledProgram::from_program was
  - It now supports Label::Placeholder, which makes sense; this leaves it to downstream consumers to error on those placeholders when not allowed. This isn't of much practical value because in most use cases today the program is serialized between construction in quil-rs and the next consumer...but in principle, this is the right thing to do.
  - Small changes in how the dot format graph is labeled and serialized.
Implements From<BasicBlock> for ProgramStats to follow the existing pattern in feat!: #334: program stats #336
- Note: this feels less than ergonomic. See below for my proposal for improvement.

What's not done here (and can be done if merged into #336):

Correct the pub interface and correctly mark PR as breaking
Documentation

Proposal for way forward on stats

first, metrics or analysis sounds more correct than stats to me; I hear the standard statistics like mean, median, etc when I hear stats

The following metrics make sense for their respective targets:

Program via ControlFlowGraph

width
uses_dynamic_control_flow: walk the blocks and look at their terminators. if any conditional jumps, then true, else false
when no dynamic control flow: then the BasicBlock metrics make sense, because they can be folded into a single value because the execution flow is known.

BasicBlock

width
swap_count
gate_volume
qubits_used -> I don't think this needs to be cached, that adds complexity and unclear value. Caching on Program was key because of how and when it was called; I'd expect this to be called exactly once in the lifetime of ProgramStats

ExecutionGraph wrapping BasicBlock

I might rename this to something more specific like QubitGraph

depth
fidelity_estimate

ScheduledBasicBlock

duration -> end time of flattened pulse program (TBD in a different, hefty PR)

None of the above

instruction_count seems unnecessary given how easy body_instructions().len() is

Summary

So, given the above proposal, I don't know that traits or wrapper structs are worth the complexity (except for ExecutionGraph). Each respective type can just implement its methods directly because even the same method might have different return types (e.g. Program#depth vs BasicBlock#depth will have different failure modes and thus errors). To get a program's depth, the user might call ExecutionGraph::from(self.get_only_basic_block()?).depth(). I don't yet have a neat picture for how the user could efficiently calculate program.depth(); program.expected_fidelity() because that needs, in the case of multiple blocks without DCF, to construct an ExecutionGraph around each BasicBlock and persist that between calls.

github-actions · 2024-02-09T07:21:31Z

PR Preview Action v1.4.7
🚀 Deployed preview to https://rigetti.github.io/quil-rs/pr-preview/pr-339/
on branch `quil-py-docs` at 2024-02-09 07:21 UTC

kalzoo · 2024-02-09T07:20:59Z

quil-rs/src/program/analysis/control_flow_graph.rs

This is a key new file

kalzoo · 2024-02-09T07:21:23Z

quil-rs/src/program/analysis/stats/mod.rs

@@ -66,15 +68,41 @@ impl InstructionsSource for Program {
    }
 }

+impl<'p> InstructionsSource for BasicBlock<'p> {


This is to fit the existing pattern, although would not be how I'd do it from scratch; see description

It seems like BasicBlock is effectively what I was trying to capture with InstructionsSource anyway.

kalzoo · 2024-02-09T07:23:13Z

quil-rs/src/program/scheduling/graph.rs

-        )?;
-
-        Ok(ScheduledProgram { blocks })
+        let control_flow_graph = program.as_control_flow_graph();


Note how much cleaner the implementation in as_control_flow_graph is than this one.

Also, that new implementation tolerates/elides more instructions than this previous one did. Some of those end up in errors within ScheduledBasicBlock because of lack of calibrations, but DEFCAL and friends are allowed now where they were once errors.

kalzoo · 2024-02-09T07:25:04Z

quil-rs/src/program/scheduling/graphviz_dot.rs

                                );
                        };
                    }
-                    BlockTerminator::Unconditional { target } => {
+                    // BasicBlockTerminator::Conditional {


todo: I want to come back to this and rework BasicBlockTerminator::JumpWhen and ::JumpUnless into a single enum similar to this previous Conditional.

Should that be considered a future todo or something to be done as part of #334?

BatmanAoD · 2024-02-09T22:24:22Z

quil-rs/src/program/analysis/control_flow_graph.rs

+    pub fn get_first_basic_block(&self) -> Option<BasicBlock> {
+        self.as_control_flow_graph().blocks.into_iter().next()
+    }


This seems like sort of a surprising thing to do: construct the entire graph, then discard most of it. Do you think this functionality will be needed outside of tests?

yeah this is not something we want to ship, but I do think it's quite useful as a convenience function to let the user treat the program like a block and get the metrics they want from it. Your new TryFrom implementation is just as good though

BatmanAoD

Seems reasonable.

What consumers are there that will need to be updated for the breaking change?

quil-rs/src/program/analysis/control_flow_graph.rs

BatmanAoD · 2024-02-09T22:30:48Z

quil-rs/src/program/analysis/stats/mod.rs

@@ -66,15 +68,41 @@ impl InstructionsSource for Program {
    }
 }

+impl<'p> InstructionsSource for BasicBlock<'p> {


It seems like BasicBlock is effectively what I was trying to capture with InstructionsSource anyway.

BatmanAoD · 2024-02-09T22:32:19Z

quil-rs/src/program/analysis/stats/mod.rs

    qubits: HashSet<&'a Qubit>,
 }

-impl<'a, 'b> From<&'a InstructionBlock<'b>> for InstructionBlockWithQubitSet<'a, 'b> {
-    fn from(block: &'a InstructionBlock<'b>) -> Self {
+impl<'a, 'b> From<&'a ScheduledBasicBlock<'b>> for InstructionBlockWithQubitSet<'a, 'b> {


Oh, InstructionBlockWithQubitSet was just a hack to let InstructionBlock work as an instruction-source.

BatmanAoD · 2024-02-09T22:34:49Z

quil-rs/src/program/scheduling/graphviz_dot.rs

                                );
                        };
                    }
-                    BlockTerminator::Unconditional { target } => {
+                    // BasicBlockTerminator::Conditional {


Should that be considered a future todo or something to be done as part of #334?

kalzoo · 2024-02-10T00:11:44Z

Seems reasonable.

What consumers are there that will need to be updated for the breaking change?

Only internal ones, to my knowledge. I can validate that before we merge #334

kalzoo added 9 commits February 8, 2024 20:38

add initial control flow graph construction

250ee4f

add Program methods for basic block retrieval

50b7436

reorganize into new analysis module

858b5a7

restructure crate

3f8f48f

impl<'p> InstructionsSource for BasicBlock<'p>

84d04f8

build ScheduledProgram from ControlFlowGraph and rename it

cadb72e

move scheduling snapshots

ecf1bed

update scheduling snapshots to reflect logical changes

691f281

chore: partial lint

24b1406

kalzoo changed the base branch from main to 334-program-stats February 9, 2024 07:20

kalzoo commented Feb 9, 2024

View reviewed changes

BatmanAoD reviewed Feb 9, 2024

View reviewed changes

BatmanAoD approved these changes Feb 9, 2024

View reviewed changes

Error message acknowledging possibility of empty program

67961f9

BatmanAoD marked this pull request as ready for review February 9, 2024 23:51

BatmanAoD merged commit 7fc8b9c into 334-program-stats Feb 9, 2024
12 of 13 checks passed

BatmanAoD deleted the 334-program-stats--kalzoo branch February 9, 2024 23:51

BatmanAoD mentioned this pull request Feb 9, 2024

Additional analysis metrics #340

Closed

kalzoo mentioned this pull request Feb 10, 2024

feat!: #334: program stats #336

Merged

kalzoo mentioned this pull request Apr 22, 2024

Compute the duration of an instruction, block, and program #325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend ProgramStats PR #339

Extend ProgramStats PR #339

kalzoo commented Feb 9, 2024

github-actions bot commented Feb 9, 2024

kalzoo Feb 9, 2024

kalzoo Feb 9, 2024

BatmanAoD Feb 9, 2024

kalzoo Feb 9, 2024

kalzoo Feb 9, 2024

BatmanAoD Feb 9, 2024

BatmanAoD Feb 9, 2024

kalzoo Feb 10, 2024

BatmanAoD left a comment

BatmanAoD Feb 9, 2024

BatmanAoD Feb 9, 2024

BatmanAoD Feb 9, 2024

kalzoo commented Feb 10, 2024

Extend ProgramStats PR #339

Extend ProgramStats PR #339

Conversation

kalzoo commented Feb 9, 2024

What's not done here (and can be done if merged into #336):

Proposal for way forward on stats

Program via ControlFlowGraph

BasicBlock

ExecutionGraph wrapping BasicBlock

ScheduledBasicBlock

None of the above

Summary

github-actions bot commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BatmanAoD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalzoo commented Feb 10, 2024