⚡️ Improve walkProgramGraph driver performance - #1930
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@burgholzer Any change you could ask codex for a review of the driver file (given the PR description)? |
denialhaag
changed the title
walkProgramGraph driver performancewalkProgramGraph driver performance
denialhaag
added
enhancement
Sure thing, will kick off a run later today when I have stable internet. |
|
🤖 AI text below 🤖 I reviewed the driver change and found that the reported speedup is confounded by a correctness regression in Before this PR, the callback's ready range was recomputed by filtering the complete A focused regression with two initially ready CNOTs demonstrates this: releasing only one must leave the other visible when the released wires expose the next gate. The PR implementation loses the deferred CNOT. A second end-to-end mapping regression, with a control-flow operation ready alongside an independent barrier chain, also exposed the consequences of persistent/deferred readiness in mapping consumers. I built a local release-mode A/B harness with deterministic frontier-heavy and routing-heavy circuits, three warmups, 25 paired samples, alternating execution order, and raw timing/statistical output:
These local workloads are not the original circuit corpus from the PR description, so they do not by themselves quantify how much of the reported ~28% aggregate result came from the behavior change. They do show that the current benchmark comparison is not valid evidence for the proposed optimization. The clean next step is therefore to revert the |
The test captures that ready operations omitted from one callback must remain visible to later callbacks until they are released. Assisted-by: GPT-5.6 via Codex
Satisfy the CI-only include-cleaner check for the test-only regression commit. Assisted-by: GPT-5.6 via Codex
Restore the ready-range behavior so unreleased ready operations remain visible to subsequent callbacks. Assisted-by: GPT-5.6 via Codex
Assisted-by: GPT-5.6 via Codex
|
9266cd8 adds a regression test that shows the flaw in the implementation of this PR. What I learned from this: We really need better and more comprehensive testing of the mapping pass so that we do not perform faulty optimizations. |
It's Saturday, but I couldn't let this go, so I simply fixed this right now and re-run the benchmarks. First of all, thanks for wasting your codex credits for fixing such an obvious bug. I hope CodeRabbit would've also been able to catch this. However, I think it's quite sad that codex didn't get the actual reason why this should be faster: Because the |
Interesting. Now the improvement also looks way more consistent than before. |
Assisted-by: GPT-5.6 via Codex
Require the mapping pass to route incompatible two-qubit interactions across every qco.index_switch region. This test fails on the current PR head because the merge dropped IndexSwitchOp dispatch. Assisted-by: GPT-5.6 via Codex
Restore IndexSwitchOp to the recursive routing dispatch after the latest main merge dropped it. Assisted-by: GPT-5.6 via Codex
Record whether a pending operation is unitary when the graph driver first classifies it. Reuse that bit in mapping callbacks instead of repeating MLIR interface lookups on the hot path. Assisted-by: GPT-5.6 via Codex
|
🤖 AI text below 🤖 Matthias's follow-up is correct: after restoring the traversal semantics, the remaining driver changes still provide a genuine performance improvement. The key is avoiding repeated MLIR interface checks on a hot path, not the earlier change in ready-operation behavior. Profiling makes that distinction visible:
During the follow-up I found a separate correctness regression introduced by the later merge commit: The final small optimization in I reran a release-mode A/B benchmark against current
An exploratory isolated A/B attributes roughly another 4.5% to the final classification cache. Route-quality samples remained overlapping for the frontier workload, while the routing workload produced exactly 171 swaps for every sample, so the timing improvement is not explained by systematically easier routes. Local validation on the final head passed all 83 QCO utility tests, all 16 mapping tests, and the full repository lint suite. Final-head CI is now running. The benchmark harness and raw artifacts remain local, as planned, so they can be refined and proposed separately without expanding this PR. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe graph walker now tracks pending operations with collected wire indices and exposes ready operations through ChangesQCO ReadyMap traversal
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProgramGraph
participant Mapping
participant Layout
ProgramGraph->>Mapping: Provide ReadyMap operation and wire-index entries
Mapping->>Layout: Check mapped qubit adjacency
Layout-->>Mapping: Return adjacency status
Mapping->>ProgramGraph: Release executable operations or defer routing
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mlir/include/mlir/Dialect/QCO/Utils/Drivers.h`:
- Around line 38-57: Update the Doxygen callback signature documentation for
WalkProgramGraphFn to replace the stale ReadyRange reference with ReadyMap,
matching the alias used by the actual function_ref type; leave the surrounding
documentation unchanged.
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 1169-1195: Deduplicate nested-region operations staged by the
walkProgramGraph callback before stack.emplace_back(). Track the operations
already added to stack during this traversal and skip subsequent entries for the
same operation, while preserving existing BarrierOp and adjacent UnitaryOp
release behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0dbde1bd-dc73-4fcc-8ba3-2f765e2b3385
📒 Files selected for processing (4)
mlir/include/mlir/Dialect/QCO/Utils/Drivers.hmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cppmlir/unittests/Dialect/QCO/Utils/test_drivers.cpp
…unich-quantum-toolkit/core into enh/improve-driver-performance
Keep implementation members distinct from parameters, localize the pending state, use DenseMap insertion directly, and iterate key views where mapped values are unused. Assisted-by: GPT-5.6 via Codex
Track nested operations when staging recursive routing so persistent ready operations are dispatched only once while independent wires advance. Cover the failure with a multi-qubit barrier-chain regression. Assisted-by: GPT-5.6 via Codex
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
These look like genuine improvements. Let's get this merged 🚀
3 participants
Description
I spent this morning investigating and profiling the mapping pass using the benchmark I recently created and noticed a few interesting bits. As a consequence I tried to optimize as much as possible - overall a ~30% reduction in runtime on the set of circuits I benchmarked on (See below for table).
Moreover, I've also learned that
isa,cast, and consequentlyTypeSwitchon interfaces is really expensive in MLIR as it always traverses a sorted array of interface libraries to find the correct context (See here for the MLIR implementation). I would expect another maybe not so insignificant improvement in runtime by swapping the UnitaryInterface withisa<XOp, YOp, ...>. To put this in numbers: It requires roughly the same amount of CPU cycles to perform the cast toUnitaryOpInterfaceas to manage the DenseMap for the pending wires.Consequently, I tried to avoid redundant
isaandcastoperation for the interface in the driver function. Additionally, I moved theisa<AllocOp, ...>elsewhere to avoid the check for each iteration step (also noticed while profiling).Benchmarks
Using 12x10 qubit grid as architecture and
Checklist
If PR contains AI-assisted content:
🤖 *AI text below* 🤖(titles are exempt).Assisted-by: [Model Name] via [Tool Name]footer.