🐛 Fix dangling references in collectQubitValuesInsideSCFOps

[li-mingbao]

Description

Refactors collectQubitValuesInsideSCFOps in the QC to QCO conversion to directly use state->maps to store the values instead of using a reference to it. This avoids dangling references when the maps get rehashed/reallocated after inserting values.

Fixes #1747

Checklist

• The pull request only contains commits that are focused and relevant to this change.
• I have added appropriate tests that cover the new/changed functionality.
• I have updated the documentation to reflect these changes.
• I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals.
• I have added migration instructions to the upgrade guide (if needed).
• The changes follow the project's style guidelines and introduce no new warnings.
• The changes are fully tested and pass the CI checks.
• I have reviewed my own code changes.

If PR contains AI-assisted content:

• I have disclosed the use of AI tools in the PR description as per our AI Usage Guidelines.
• AI-assisted commits include an Assisted-by: [Model Name] via [Tool Name] footer.
• I confirm that I have personally reviewed and understood all AI-generated content, and accept full responsibility for it.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[li-mingbao]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR refactors the collectQubitValuesInsideSCFOps static helper function to fix a memory corruption crash during QC-to-QCO conversion. The function now accumulates qubits and registers into local sets before committing to state maps, uses region-scoped definition checks, and properly handles memref::LoadOp cases.

Changes

SCF Region Collection Logic

Layer / File(s)	Summary
Rewritten collectQubitValuesInsideSCFOps with local accumulation mlir/lib/Conversion/QCToQCO/QCToQCO.cpp	Refactored collection strategy from immediate state map mutation during traversal to accumulating qubits/registers in local sets, checking definitions in region-scoped qubitInfoMap, merging recursive results into locals, and updating memref::LoadOp to populate region qubit info before final union into state maps.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• munich-quantum-toolkit/core#1700: Both PRs update SCF qubit handling in mlir/lib/Conversion/QCToQCO/QCToQCO.cpp by changing how state->regionQubitMap/register requirements are determined/used, so the main PR's rewritten collectQubitValuesInsideSCFOps logic directly impacts the retrieved PR's insertion/extraction policy keyed off state.regionQubitMap[target].

Suggested labels

fix, c++, MLIR

Suggested reviewers

• burgholzer

Poem

A rabbit refactored the maps today, 🐰
Local sets gather, then pack away,
No more mutations mid-traversal's dance,
Just careful unions in their rightful stance,
The malloc crash now takes its final say. 🔧

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The PR description identifies the issue fixed (#1747), explains the refactoring rationale (avoiding dangling references), and provides context. However, the checklist items are all unchecked with no indication of test coverage, documentation updates, or changelog entries being addressed.	Clarify which checklist items have been completed. At minimum, confirm that tests have been added for the fix, documentation updated if needed, and changelog entries created for this bug fix.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing dangling references in the collectQubitValuesInsideSCFOps function.
Linked Issues check	✅ Passed	The PR directly addresses issue #1747 by refactoring collectQubitValuesInsideSCFOps to store values locally before inserting into regionMaps, which prevents dangling references from map reallocation during the QC to QCO conversion that was causing the crash.
Out of Scope Changes check	✅ Passed	All changes are scoped to the collectQubitValuesInsideSCFOps function in QCToQCO.cpp, directly addressing the dangling reference issue in #1747 without introducing unrelated modifications.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

✨ Simplify code

• Create PR with simplified 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 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/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 521-525: The local-definition check (isDefinedLocally) only
consults qubitInfoMap and misses values defined by ops inside the same region
(e.g., qc.alloc), causing region-local qubits/memrefs to be misclassified as
external; update that lambda to also consider a Value as local if its defining
operation exists and its defining operation's region
(definingOp->getParentRegion() or op->getParentRegion()) equals the region
argument, and apply the same logic to the memref-local checks that reference
memrefInfoMap (and any similar predicates around Lines 541–565) so both qubit
and memref lookups first accept values defined in the same region regardless of
whether they were recorded in the maps.

🪄 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

Run ID: fbe983ae-bf5a-489e-ae7c-a455a5e4d5af

📥 Commits

Reviewing files that changed from the base of the PR and between af09841 and b9d114f.

📒 Files selected for processing (1)

• mlir/lib/Conversion/QCToQCO/QCToQCO.cpp

[li-mingbao]

@MatthiasReumann I think this should fix the issue. Let me know if there are any other problems.

Btw: I have no idea what I did with the CI but there seems to be something broken. Do you know what this is?

[burgholzer]

I pushed a commit that simplifies the diff a bit more and reduces the number of map lookups.

@li-mingbao Could you please add a test to the test suite covering this change? The test should fail before the commit and succeed afterwards.

[burgholzer]

LGTM now! Thanks! 🙏