Skip to content

fix: Dataflow analysis produces unsound results on Hugrs with entrypoint #2255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
acl-cqc merged 5 commits into from
Jun 3, 2025
Merged

fix: Dataflow analysis produces unsound results on Hugrs with entrypoint #2255

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9f4bcbb
Test to demonstrate issue #2254
acl-cqc May 26, 2025
6f2121f
dataflow analysis runs on the whole Hugr to generate results for entr…
acl-cqc May 26, 2025
7ea2c16
Make sameness/difference of results more obvious
acl-cqc May 26, 2025
d9bd2dc
clarify doc+impl of Machine::run
acl-cqc May 26, 2025
6174928
Merge remote-tracking branch 'origin/main' into acl/dataflow_root
acl-cqc Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions hugr-passes/src/dataflow/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,22 +442,21 @@ fn test_region() {
let mut builder = DFGBuilder::new(Signature::new(vec![bool_t()], vec![bool_t(); 2])).unwrap();
let [in_w] = builder.input_wires_arr();
let cst_w = builder.add_load_const(Value::false_val());
// Create a nested DFG which gets in_w passed as an input, but has a nonlocal edge
// from the LoadConstant
let nested = builder
.dfg_builder(Signature::new_endo(vec![bool_t(); 2]), [in_w, cst_w])
.dfg_builder(Signature::new(bool_t(), vec![bool_t(); 2]), [in_w])
.unwrap();
let nested_ins = nested.input_wires();
let nested = nested.finish_with_outputs(nested_ins).unwrap();
let [nested_in] = nested.input_wires_arr();
let nested = nested.finish_with_outputs([nested_in, cst_w]).unwrap();
let hugr = builder.finish_hugr_with_outputs(nested.outputs()).unwrap();
let [nested_input, _] = hugr.get_io(nested.node()).unwrap();
let whole_hugr_results = Machine::new(&hugr).run(TestContext, [(0.into(), pv_true())]);
assert_eq!(
whole_hugr_results.read_out_wire(Wire::new(nested_input, 0)),
Some(pv_true())
);
assert_eq!(
whole_hugr_results.read_out_wire(Wire::new(nested_input, 1)),
Some(pv_false())
);
assert_eq!(whole_hugr_results.read_out_wire(cst_w), Some(pv_false()));
assert_eq!(
whole_hugr_results.read_out_wire(Wire::new(hugr.entrypoint(), 0)),
Some(pv_true())
Expand All @@ -467,16 +466,22 @@ fn test_region() {
Some(pv_false())
);

// Do not provide a value on the second input (constant false in the whole hugr, above)
Copy link
Contributor Author

@acl-cqc acl-cqc May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not testing what was intended: not passing the second input, just makes Machine::run / Machine::prepopulate_inputs provide Top on the missing input, so there is no connection with (or reliance on the edge from) the graph outside the entrypoint. Hence, replacing the second input with a nonlocal edge.

let sub_hugr_results =
Machine::new(hugr.with_entrypoint(nested.node())).run(TestContext, [(0.into(), pv_true())]);
assert_eq!(
sub_hugr_results.read_out_wire(Wire::new(nested_input, 0)),
Some(pv_true())
);
assert_eq!(sub_hugr_results.read_out_wire(cst_w), None);
assert_eq!(
sub_hugr_results.read_out_wire(Wire::new(nested_input, 1)),
Some(PartialValue::Top)
sub_hugr_results.read_out_wire(Wire::new(nested.node(), 0)),
Some(pv_true())
);
// `Bottom` is a terrible (unsound!) result to get here for a Wire within the desired region.
// See https://github.com/CQCL/hugr/issues/2254.
assert_eq!(
sub_hugr_results.read_out_wire(Wire::new(nested.node(), 1)),
Some(PartialValue::Bottom)
);
for w in [0, 1] {
assert_eq!(
Expand Down