Preserve constant loop bounds through adjoint-lowering#2959
Conversation
Relocate the forward pass into the same location as the reverse pass, as having the two pieces of the pass in different locations is very confusing. A subdirectory is introduced for the pass for better organization.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2959 +/- ##
==========================================
+ Coverage 96.95% 96.97% +0.02%
==========================================
Files 166 166
Lines 19143 19209 +66
Branches 1772 1788 +16
==========================================
+ Hits 18560 18628 +68
+ Misses 430 429 -1
+ Partials 153 152 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
joeycarter
left a comment
joeycarter
left a comment
There was a problem hiding this comment.
Looks good, just a few editorial things and questions—nothing blocking. 👍
| // CHECK: catalyst.list_push %arg0 | ||
| // CHECK: [[stop:%.+]] = catalyst.list_pop | ||
| // CHECK: [[for_out:%.+]]:2 = scf.for {{%.+}} = [[start]] to [[stop]] step [[step]] | ||
| %8:2 = scf.for %arg3 = %c0 to %stop step %c1 iter_args(%arg5 = %arg1, %arg6 = %arg2) -> (!quantum.bit, !quantum.bit) { | ||
| %out_qubits:2 = quantum.custom "gate1"() %arg5, %arg6 : !quantum.bit, !quantum.bit | ||
| %out_qubits_0:2 = quantum.custom "gate2"() %out_qubits#0, %out_qubits#1 : !quantum.bit, !quantum.bit | ||
| scf.yield %out_qubits_0#0, %out_qubits_0#1 : !quantum.bit, !quantum.bit |
There was a problem hiding this comment.
The previous test checked that the order of gate1 and gate2 was reversed and that the adj unit attribute was added. Is it worth doing that here as well, or would it be redundant?
There was a problem hiding this comment.
The reason is that the previous one was an existing test checked the action of the adjoint pass (the reversing you mentioned). This PR is only about preserving constants, which is the behaviour I'm unit testing here, the pass action is otherwise not relevant.
To be fair I also added these checks to the existing test, it might have been cleaner to create two new ones I suppose.
There was a problem hiding this comment.
Not critical, up to you.
| // TODO: Generalize to arbitrary dimensions | ||
| if (2 != shape.size()) { | ||
| op->emitOpError() << "Caching only supports tensors complex F64"; | ||
| } | ||
| // TODO: Generalize to other types | ||
| Type elementType = aTensorType.getElementType(); | ||
| if (!isa<ComplexType>(elementType)) { | ||
| op->emitOpError() << "Caching only supports tensors complex F64"; | ||
| } | ||
| // TODO: Generalize to other types | ||
| Type f64 = cast<ComplexType>(elementType).getElementType(); | ||
| if (!f64.isF64()) { | ||
| op->emitOpError() << "Caching only supports tensors complex F64"; | ||
| } |
There was a problem hiding this comment.
[Optional] All the error messages are the same; could these checks be condensed into a single if statement? Or is this just to maintain a bit more flexibility in the future?
There was a problem hiding this comment.
I touched as little logic as I could, if you go commit by commit it's a bit easier to see (first one just moves files, second spits them up a bit differently (with minor content edits, but in theory non-functional), third has the actual changes). This was relocated from QuantumSplitting.cpp without change.
That being said, your question is probably legitimate.
There was a problem hiding this comment.
Up to you, the gain in refactoring these if blocks is pretty small.
Co-authored-by: Joey Carter <joey.snarrcarter@gmail.com>
3 participants
As laid out in #2936, the
--adjoint-loweringpass converts all classical values used by the reverse pass to dynamic ones via a tape caching mechanism (LIFOlist.push/pop). This hinders analyzability for example inqp.specs. This PR skips the tape mechanism for constant loop bounds.As a pre-requisite to implementing the change, the pass is restructured into a more coherent structure (all in the same directory) of pass driver (
AdjointLowering.cpp), forward pass / classical value caching (Forward.cpp), reverse pass / inverse quantum instruction stream & adjoint propagation (Reverse.cpp), and a helper data structure (QuantumCache.cpp/hpp). The actual functional change is very small (2f6ad9e).closes #2936
[sc-122139]
Future improvements: One could apply this fix to any constant classical value cached by the tape.
Additional note: To "fix" the example in the issue one should run the canonicalization pass before the adjoint lowering (standard in our pipeline) so that the detection of constant values is facilitated.