Decomp/qref decomposable interface - #3161
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3161 +/- ##
=======================================
Coverage 95.65% 95.65%
=======================================
Files 177 177
Lines 21011 21011
Branches 2117 2117
=======================================
Hits 20098 20098
Misses 725 725
Partials 188 188 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
@kipawaa Could you provide more info on this? |
There's at least two points in the current graph decomp pass that's very inefficient.
Issue 1 I'm very confident is non existent if the pass were in reference semantics. So at least there's that. We have too many separate implementations of the walkback, and each of these must be maintained separately (e.g. bug found in #3156 ). Any pass that is walking back should just be in reference semantics since the "walkback" in the semantics conversion pass is the most tested! Issue 2 might be more tricky. The rules are generated from the frontend as quantum subroutines, which takes in wires as (a tensor of) integers (aka "extract indices). I would really love it if the frontend can generate rules that act on |
@paul0403 Made the major points above, but to add to this is that the on-demand rules (and truly all rules) are initially lowered in reference semantics and then have to be converted to value semantics for compatibility with the passes. I think it would be nice to migrate the entire graph-decomps system to reference semantics for all of these reasons. We could then place the rules in their own module separate from the user circuit, and this way they can be isolated from the user program and remain in reference semantics throughout their lifetime. |
|
Nice work @yde773786! Sorry to spring this on you right after you finished the work, but could you update to accommodate controls that were introduced in #3127? |
|
Added the tests. I found no need to make any changes, so ported identical functionality/tests from Did want to bring attention to this test again, though (since it's a test that is not present in the |
8af2e84 to
ebf9f03
Compare
kipawaa
left a comment
There was a problem hiding this comment.
Great work, thanks @yde773786!
ebf9f03 to
3fd9dd7
Compare
| } | ||
|
|
||
| template <typename T, typename PrintFunc> | ||
| void printSortedMap(const llvm::StringMap<T> &map, llvm::raw_string_ostream &ss, |
There was a problem hiding this comment.
wrapModifiers, printSortedMap, printDynamicShape and printWireLens are all copies that are not specific to the either dialect, and should be factored out
There was a problem hiding this comment.
Since it's tracked, let's tackle it in a follow up PR.
There was a problem hiding this comment.
I don't think an interface is necessary here, this is just about factoring out duplicate helper functions. But please do tackle this in a follow up!
There was a problem hiding this comment.
I think the idea is to put the interface itself in the Catalyst dialect, which means all these helpers would exist just once in Catalyst dialect too.
There was a problem hiding this comment.
Although I don't necessarily agree with putting quantum things in Catalyst, I can't really think of a better place 😅
There was a problem hiding this comment.
Yeah Catalyst doesn't sound like the right place for a quantum related interface, it should just go into quantum. Regarding the helpers, if they are moved to the interface that's fine too, I'm just saying all that was necessary here was moving them to a header from which they can be imported into both dialects.
|
I am branching off this PR to migrate |
16f4b7b to
1427346
Compare
maliasadi
left a comment
There was a problem hiding this comment.
Looks good! Thanks @yde773786
… for gates that can be decomposable. Ensure closely followed implementation as in Quantum dialect
… decomposableGateInterface in Quantum dialect
Changelog generalized to include all PRs that implement DecomposableGateInterface across dialects Co-authored-by: Ali Asadi <10773383+maliasadi@users.noreply.github.com>
…ate required in all corresponding DecompGate Interfaces
… GOID, into QRef. Update tests as well.
Co-authored-by: Ali Asadi <10773383+maliasadi@users.noreply.github.com>
ecf06c6 to
bffe31d
Compare
Context:
The
DecomposableGateInterface was introduced to provide a generic interface for decomposition of quantum gates. We want to migrate thegraph-decompositionanddecompose-loweringpasses to reference semantics, but this will require theDecomposableGateinterface be implemented for reference semantics gatesDescription of the Change:
The
DecomposableGateInterface is added for theQrefdialect, and the decomposable gates inherit this interface appropriately. The format used is identical to how the interface is defined and used in theQuantumdialect.gtestfor this interface addition added.Using MLIR types in the GOID as defined here is used.
A corresponding test that should fail with the previous (python style types in GOID) is also added to the
Qrefdialect'sDecomposableGateInterfacegtests.Benefits:
Unblocks implementation of
graph-decompositionanddecompose-loweringpasses in reference semantics.Possible Drawbacks:
Code duplication exists with the
Quantumdialect's implementation ofDecomposableGate. Modifications need to be applied in both places.Related GitHub Issues:
[sc-128397]