Skip to content

Use optimized AND for decomposition #1202

Merged
swernli merged 4 commits into
mainfrom
swernli/optimize-decomp
Feb 27, 2024
Merged

Use optimized AND for decomposition #1202
swernli merged 4 commits into
mainfrom
swernli/optimize-decomp

Conversation

@swernli
Copy link
Copy Markdown
Collaborator

@swernli swernli commented Feb 26, 2024

This allows the "unrestricted" configuration to use an optimized AND for multi-controlled decomposition that relies on measurement-based adjoint. The existing PhaseCCX will still be used for "base" configuration since branching measurement is not allowed there.

Fixes #1137

This allows the "unrestricted" configuration to use an optimized `AND` for multi-controlled decomposition that relies on measurment-based adjoint. The existing `PhaseCCX` will still be used for "base" configuration since branching measurement is not allowed there.

Fixes #1137
@github-actions
Copy link
Copy Markdown

Change in memory usage detected by benchmark.

Memory Report for 5675de4

Test This Branch On Main
compile core + standard lib 15964749 bytes 15946449 bytes

@github-actions
Copy link
Copy Markdown

Benchmark for 5675de4

Click to view benchmark
Test Base PR %
Array append evaluation 756.4±6.34µs 756.9±13.73µs +0.07%
Array literal evaluation 521.2±71.93µs 536.6±58.35µs +2.95%
Array update evaluation 753.7±6.08µs 753.4±13.34µs -0.04%
Deutsch-Jozsa evaluation 30.4±0.16ms 7.0±0.05ms -76.97%
Large file parity evaluation 35.3±0.19ms 35.2±0.36ms -0.28%
Large input file 40.4±0.85ms 40.3±1.74ms -0.25%
Large nested iteration 74.6±2.91ms 75.7±0.67ms +1.47%
Standard library 23.1±0.50ms 23.0±0.76ms -0.43%
Teleport evaluation 95.3±1.62µs 93.6±2.14µs -1.78%

Copy link
Copy Markdown
Member

@msoeken msoeken left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you.

We could replace M.Q.Unstable.Arithmetic.ApplyAndAssuming0Target by AND in this PR.

Further, I suggest to consider making AND a public operation.

@swernli
Copy link
Copy Markdown
Collaborator Author

swernli commented Feb 26, 2024

Looks good to me, thank you.

We could replace M.Q.Unstable.Arithmetic.ApplyAndAssuming0Target by AND in this PR.

Further, I suggest to consider making AND a public operation.

We'll definitely be discussing when/how to expose this as a public API. For now though, we can't have them both use the same utility function because they are in different namespaces and cannot see the callables marked as internal in the other.

@msoeken
Copy link
Copy Markdown
Member

msoeken commented Feb 26, 2024

Looks good to me, thank you.
We could replace M.Q.Unstable.Arithmetic.ApplyAndAssuming0Target by AND in this PR.
Further, I suggest to consider making AND a public operation.

We'll definitely be discussing when/how to expose this as a public API. For now though, we can't have them both use the same utility function because they are in different namespaces and cannot see the callables marked as internal in the other.

We could make it public in the unstable namespace to avoid duplication before discussing the public API in a non-unstable namespace.

@swernli
Copy link
Copy Markdown
Collaborator Author

swernli commented Feb 26, 2024

We could make it public in the unstable namespace to avoid duplication before discussing the public API in a non-unstable namespace.

That's certainly possible, yes. I just would feel a bit awkward about having most callables in the Microsoft.Quantum.Intrinsic namespace depend on something from an unstable namespace. Feels a bit like an inversion...

The other concern I have is with the adjoint and how it is used, but that might be a longer conversation than just for this PR. What I'd like is to get this merged and then open an issue about making AND a supported intrinsic in QIR and public in Q#. Then we can get into the details of the design in that issue.

@github-actions
Copy link
Copy Markdown

Change in memory usage detected by benchmark.

Memory Report for ee2263c

Test This Branch On Main
compile core + standard lib 15964749 bytes 15946449 bytes

@github-actions
Copy link
Copy Markdown

Benchmark for ee2263c

Click to view benchmark
Test Base PR %
Array append evaluation 762.0±39.03µs 759.8±5.17µs -0.29%
Array literal evaluation 507.2±24.28µs 513.8±39.05µs +1.30%
Array update evaluation 756.1±7.47µs 758.8±24.12µs +0.36%
Deutsch-Jozsa evaluation 30.4±0.16ms 7.0±0.05ms -76.97%
Large file parity evaluation 35.1±0.12ms 35.1±0.49ms 0.00%
Large input file 42.0±1.14ms 41.7±1.05ms -0.71%
Large nested iteration 75.1±0.78ms 77.2±1.83ms +2.80%
Standard library 23.9±0.74ms 23.6±0.63ms -1.26%
Teleport evaluation 94.6±2.22µs 95.2±2.15µs +0.63%

Copy link
Copy Markdown
Contributor

@DmitryVasilevsky DmitryVasilevsky left a comment

Choose a reason for hiding this comment

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

Approving library change.

@swernli swernli added this pull request to the merge queue Feb 27, 2024
Merged via the queue into main with commit 5758cb1 Feb 27, 2024
@swernli swernli deleted the swernli/optimize-decomp branch February 27, 2024 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modern ApplyControlledOnInt implementation less efficient for multiple controls compared to classic ControlledOnInt

4 participants