feat: Lower "Drop" op in guppy extension, update hugr minor version #979
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #979 +/- ##
==========================================
- Coverage 79.65% 79.40% -0.25%
==========================================
Files 94 95 +1
Lines 11552 11592 +40
Branches 11282 11322 +40
==========================================
+ Hits 9202 9205 +3
- Misses 1720 1754 +34
- Partials 630 633 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tket2-hseries/src/lower_drops.rs
Outdated
| fn run(&self, hugr: &mut H) -> Result<Self::Result, Self::Error> { | ||
| let mut rt = ReplaceTypes::default(); | ||
| rt.linearizer() | ||
| .register_callback(array_type_def(), copy_discard_array); |
There was a problem hiding this comment.
This should possibly be done in DelegatingLinearizer::default(), which currently adds only linearize_array (for ValueArray). Or, perhaps that change happens when we remove ValueArray?
I also worry that there are other types we need to add (i.e. anything guppy-affine but hugr-linear)....however I think if there were any, we would need them in the linearize_arrays pass, and the only linearizer callback I see there is...this one!
There was a problem hiding this comment.
I am also adding the same callback in the replace borrow arrays pass - does it cause issues with multiple passes adding this?
It does seem a bit confusing to have some logic for this in the DelegatingLinearizer default and some in the passes given not all the passes may be run but if at least on pass with ReplaceTypes with linearisation is run it will use all the defaults?
There was a problem hiding this comment.
Each ReplaceTypes has its own (independent) list of callbacks, and the linearize_array callback is stateless, so it should work. But it is more of a maintenance issue - i.e. we are now getting up to 3 passes.
If multiple ReplaceTypes passes actually get run, we'll probably need to make sure linearize_array is added to all of them (not just one) - each pass only runs linearization on the parts of the Hugr that it changes.
So, might be better to add this to ReplaceTypes::default() now (as a Hugr PR, for the hugr-v0.22 release), and then one of us can remove the existing tket register_callback rather than add another....it's now a very simple Hugr PR!
There was a problem hiding this comment.
This done in hugr-0.22.1, to which I'm updating here, so you should no longer need it in the pass for borrow-arrays @tatiana-s
tket2-hseries/src/lower_drops.rs
Outdated
| fn run(&self, hugr: &mut H) -> Result<Self::Result, Self::Error> { | ||
| let mut rt = ReplaceTypes::default(); | ||
| rt.linearizer() | ||
| .register_callback(array_type_def(), copy_discard_array); |
There was a problem hiding this comment.
I am also adding the same callback in the replace borrow arrays pass - does it cause issues with multiple passes adding this?
It does seem a bit confusing to have some logic for this in the DelegatingLinearizer default and some in the passes given not all the passes may be run but if at least on pass with ReplaceTypes with linearisation is run it will use all the defaults?
| let h = std::mem::take(dfb.hugr_mut()); | ||
| Some(NodeTemplate::CompoundOp(Box::new(h))) | ||
| }, | ||
| ReplacementOptions::default().with_linearization(true), |
There was a problem hiding this comment.
Does this handle adding a relevant discard op to the type - if so how does it know which one is needed?
There was a problem hiding this comment.
Yes, the replacement for Drop is an empty DFG with the same signature as the Drop. So inside that DFG there will be an Input node, with a single outport of the relevant type, and the linearizer will figure out how to implement a discard (0-ary copy) op from that. (See CQCL/hugr#2435)
acl-cqc
changed the title
acl-cqc
changed the title
acl-cqc
changed the title
…979) Add LowerDropsPass, use in QSystemPass. hugr 0.22.0 -> 0.22.1 gets the "copy_discard_array" being in the default ReplaceTypes
Add LowerDropsPass, use in QSystemPass.
hugr 0.22.0 -> 0.22.1 gets the "copy_discard_array" being in the default ReplaceTypes