Skip to content

[core] Fix some more memtoreg issues. - #5293

Merged
schweitzpgi merged 6 commits into
NVIDIA:mainfrom
schweitzpgi:ch-memtoreg
Aug 27, 2026
Merged

[core] Fix some more memtoreg issues.#5293
schweitzpgi merged 6 commits into
NVIDIA:mainfrom
schweitzpgi:ch-memtoreg

Conversation

@schweitzpgi

Copy link
Copy Markdown
Collaborator
  • regressions in handling classical values discovered and fixed
  • add a canonical ordering to arguments appending
  • add missing guards that resulted in crashes
  • add new regression tests

@github-actions github-actions Bot added python-lang Anything related to the Python CUDA Quantum language implementation core compiler labels Aug 26, 2026
@schweitzpgi

Copy link
Copy Markdown
Collaborator Author

Fixes #5286

@sacpis sacpis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @schweitzpgi. Left few comments.

Comment thread cudaq/lib/Optimizer/Transforms/MemToReg.cpp Outdated
Comment thread cudaq/lib/Optimizer/Transforms/MemToReg.cpp
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

CI Summary (push) — ✅ passed

Run #33116907599 · ✅ 7 · ⏩ 7 · ❌ 0 · ⛔ 0

Top-level jobs (14)
Job Result
binaries ⏩ skipped
build_and_test ✅ success
changes ✅ success
config_devdeps ✅ success
config_source_build ⏩ skipped
config_wheeldeps ✅ success
devdeps ✅ success
docker_image ⏩ skipped
gen_code_coverage ⏩ skipped
metadata ✅ success
python_metapackages ⏩ skipped
python_wheels ⏩ skipped
source_build ⏩ skipped
wheeldeps ✅ success
⏩ Skipped jobs (7) — intentionally skipped on PR builds; run on merge_group / workflow_dispatch
Job
binaries
config_source_build
docker_image
gen_code_coverage
python_metapackages
python_wheels
source_build
All sub-jobs (43) — every matrix leg, with links
Job Status Link
Build and test (amd64, gcc12, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, gcc12, openmpi) / Dev environment (Python) ✅ success view
Build and test (amd64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, llvm, openmpi) / Dev environment (Python) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Python) ✅ success view
CI Summary ❔ in_progress view
Check for stable CUDA-Q changes ✅ success view
Configure build (devdeps) ✅ success view
Configure build (source_build) ⏩ skipped view
Configure build (wheeldeps) ✅ success view
Create CUDA Quantum installer ⏩ skipped view
Create Docker images ⏩ skipped view
Create Python metapackages ⏩ skipped view
Create Python wheels ⏩ skipped view
Gen code coverage ⏩ skipped view
Load dependencies (amd64, gcc12) / Caching ✅ success view
Load dependencies (amd64, gcc12) / Finalize ✅ success view
Load dependencies (amd64, gcc12) / Metadata ✅ success view
Load dependencies (amd64, llvm) / Caching ✅ success view
Load dependencies (amd64, llvm) / Finalize ✅ success view
Load dependencies (amd64, llvm) / Metadata ✅ success view
Load dependencies (arm64, gcc12) / Caching ✅ success view
Load dependencies (arm64, gcc12) / Finalize ✅ success view
Load dependencies (arm64, gcc12) / Metadata ✅ success view
Load dependencies (arm64, llvm) / Caching ✅ success view
Load dependencies (arm64, llvm) / Finalize ✅ success view
Load dependencies (arm64, llvm) / Metadata ✅ success view
Load source build cache ⏩ skipped view
Load wheel dependencies (amd64, 12.6) / Caching ✅ success view
Load wheel dependencies (amd64, 12.6) / Finalize ✅ success view
Load wheel dependencies (amd64, 12.6) / Metadata ✅ success view
Load wheel dependencies (amd64, 13.0) / Caching ✅ success view
Load wheel dependencies (amd64, 13.0) / Finalize ✅ success view
Load wheel dependencies (amd64, 13.0) / Metadata ✅ success view
Load wheel dependencies (arm64, 12.6) / Caching ✅ success view
Load wheel dependencies (arm64, 12.6) / Finalize ✅ success view
Load wheel dependencies (arm64, 12.6) / Metadata ✅ success view
Load wheel dependencies (arm64, 13.0) / Caching ✅ success view
Load wheel dependencies (arm64, 13.0) / Finalize ✅ success view
Load wheel dependencies (arm64, 13.0) / Metadata ✅ success view
Prepare cache clean-up ❔ in_progress view
Retrieve PR info ✅ success view
✅ Required checks (6/6) — declared in .github/required-checks.yml for push
Required check Status Link
Build and test (amd64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, llvm, openmpi) / Dev environment (Python) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Python) ✅ success view
Build and test (amd64, gcc12, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, gcc12, openmpi) / Dev environment (Python) ✅ success view

@khalatepradnya khalatepradnya left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, @schweitzpgi for the updates!
I tested the failing test on my branch with this change, and it is working.

nvq++ --target quantinuum --emulate  ../targettests/execution/estimate_resources_break.cpp && ./a.out
Total # of gates: 32, total # of qubits: 1, circuit depth: 32, multi-Q gate count: 0, multi-Q depth: 0
{ 
  reset :  10
  mz :  10
  x :  11
  h :  1
}

@khalatepradnya khalatepradnya left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is it possible to enable the test in issue #5286?

// XFAIL: *
// FIXME: https://github.com/NVIDIA/cuda-quantum/issues/5286

@schweitzpgi

Copy link
Copy Markdown
Collaborator Author

Is it possible to enable the test in issue #5286?

// XFAIL: *
// FIXME: https://github.com/NVIDIA/cuda-quantum/issues/5286

Let me check...

@schweitzpgi
schweitzpgi disabled auto-merge August 27, 2026 19:00
@schweitzpgi

Copy link
Copy Markdown
Collaborator Author

Is it possible to enable the test in issue #5286?

// XFAIL: *
// FIXME: https://github.com/NVIDIA/cuda-quantum/issues/5286

Let me check...

It looks like this was disabled because of how long it took it CI. I won't mess with it in this PR. That's an unrelated issue.

- regressions in handling classical values discovered and fixed
- add a canonical ordering to arguments appending
- add missing guards that resulted in crashes
- add new regression tests

Signed-off-by: Eric Schweitz <eschweitz@nvidia.com>
Signed-off-by: Eric Schweitz <eschweitz@nvidia.com>
Signed-off-by: Eric Schweitz <eschweitz@nvidia.com>
Signed-off-by: Eric Schweitz <eschweitz@nvidia.com>

@atgeller atgeller left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

Signed-off-by: Pradnya Khalate <pkhalate@nvidia.com>
@khalatepradnya

khalatepradnya commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Is it possible to enable the test in issue #5286?

// XFAIL: *
// FIXME: https://github.com/NVIDIA/cuda-quantum/issues/5286

Let me check...

It looks like this was disabled because of how long it took it CI. I won't mess with it in this PR. That's an unrelated issue.

It passed in the latest CI (Ref: logs)

Unexpectedly Passed Tests (1):
  CUDAQ-Target :: execution/estimate_resources_break.cpp

I have pushed a commit to enable it.

@atgeller atgeller left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I've found a regression from main in a simple test case with a quantum and classical variable:

func.func @min(%c: i1, %k: i32) {
  %q = quake.alloca !quake.ref
  %m = cc.alloca i32
  cc.if (%c) {
    %v = cc.load %m : !cc.ptr<i32>
    cc.store %k, %m : !cc.ptr<i32>
    quake.x %q : (!quake.ref) -> ()
  }
  quake.y %q : (!quake.ref) -> ()
  %f = cc.load %m : !cc.ptr<i32>
  quake.log_output %f : (i32) -> () {compilerGenerated}
  quake.dealloc %q : !quake.ref
  return
}

With cudaq-opt --memtoreg fails with

test.qke:4:3: error: 'cc.if' op along control flow edge from Operation cc.continue to parent: successor operand type #0 '!quake.wire' should match successor input type #0 'i32'
  cc.if (%c) {
  ^
test.qke:4:3: note: see current operation: 
%2:2 = "cc.if"(%arg0, %0) ({
^bb0(%arg3: !quake.wire):
  %4 = "quake.x"(%arg3) <{operandSegmentSizes = array<i32: 0, 0, 1>}> : (!quake.wire) -> !quake.wire
  "cc.continue"(%4, %arg1) : (!quake.wire, i32) -> ()
}, {
^bb0(%arg2: !quake.wire):
  "cc.continue"(%arg2, %1) : (!quake.wire, i32) -> ()
}) : (i1, !quake.wire) -> (i32, !quake.wire)
test.qke:4:3: note: region branch point

This works on main.

Signed-off-by: Sachin Pisal <spisal@nvidia.com>
@sacpis

sacpis commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

I've found a regression from main in a simple test case with a quantum and classical variable:

func.func @min(%c: i1, %k: i32) {
  %q = quake.alloca !quake.ref
  %m = cc.alloca i32
  cc.if (%c) {
    %v = cc.load %m : !cc.ptr<i32>
    cc.store %k, %m : !cc.ptr<i32>
    quake.x %q : (!quake.ref) -> ()
  }
  quake.y %q : (!quake.ref) -> ()
  %f = cc.load %m : !cc.ptr<i32>
  quake.log_output %f : (i32) -> () {compilerGenerated}
  quake.dealloc %q : !quake.ref
  return
}

With cudaq-opt --memtoreg fails with

test.qke:4:3: error: 'cc.if' op along control flow edge from Operation cc.continue to parent: successor operand type #0 '!quake.wire' should match successor input type #0 'i32'
  cc.if (%c) {
  ^
test.qke:4:3: note: see current operation: 
%2:2 = "cc.if"(%arg0, %0) ({
^bb0(%arg3: !quake.wire):
  %4 = "quake.x"(%arg3) <{operandSegmentSizes = array<i32: 0, 0, 1>}> : (!quake.wire) -> !quake.wire
  "cc.continue"(%4, %arg1) : (!quake.wire, i32) -> ()
}, {
^bb0(%arg2: !quake.wire):
  "cc.continue"(%arg2, %1) : (!quake.wire, i32) -> ()
}) : (i1, !quake.wire) -> (i32, !quake.wire)
test.qke:4:3: note: region branch point

This works on main.

Pushed a fix with 023be4e. Thanks @atgeller.

@sacpis

sacpis commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

I've found a regression from main in a simple test case with a quantum and classical variable:

func.func @min(%c: i1, %k: i32) {
  %q = quake.alloca !quake.ref
  %m = cc.alloca i32
  cc.if (%c) {
    %v = cc.load %m : !cc.ptr<i32>
    cc.store %k, %m : !cc.ptr<i32>
    quake.x %q : (!quake.ref) -> ()
  }
  quake.y %q : (!quake.ref) -> ()
  %f = cc.load %m : !cc.ptr<i32>
  quake.log_output %f : (i32) -> () {compilerGenerated}
  quake.dealloc %q : !quake.ref
  return
}

With cudaq-opt --memtoreg fails with

test.qke:4:3: error: 'cc.if' op along control flow edge from Operation cc.continue to parent: successor operand type #0 '!quake.wire' should match successor input type #0 'i32'
  cc.if (%c) {
  ^
test.qke:4:3: note: see current operation: 
%2:2 = "cc.if"(%arg0, %0) ({
^bb0(%arg3: !quake.wire):
  %4 = "quake.x"(%arg3) <{operandSegmentSizes = array<i32: 0, 0, 1>}> : (!quake.wire) -> !quake.wire
  "cc.continue"(%4, %arg1) : (!quake.wire, i32) -> ()
}, {
^bb0(%arg2: !quake.wire):
  "cc.continue"(%arg2, %1) : (!quake.wire, i32) -> ()
}) : (i1, !quake.wire) -> (i32, !quake.wire)
test.qke:4:3: note: region branch point

This works on main.

Pushed a fix with 023be4e. Thanks @atgeller.

BTW, the root cause here was that canonicalizeArgumentOrder permuted region block arguments but left liveOutSet (which fixes the parent's appended result order) untouched. Two sorts patched over the gap and could not for cc.if, whose LinearTypeArgs trait keeps classical values out of block arguments entirely, yielding results (i32, wire) against cc.continue operands (wire, i32).

To fix this issue, we did reorder liveOutSet (and rebuild liveInArgs) to the canonical order, so results, terminator operands, and block arguments share one order. Also, dropped both sorts and the exitSiblingSuccessors map.

@atgeller atgeller left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix @sacpis! Working now 😄

@schweitzpgi
schweitzpgi added this pull request to the merge queue Aug 27, 2026
@github-actions

Copy link
Copy Markdown

CI Summary (merge_group) — ✅ passed

Run #33120401868 · ✅ 5 · ⏩ 9 · ❌ 0 · ⛔ 0

Fast merge queue — only the llvm dev-environment build+test legs ran.
Wheels, metapackages, installers, Docker images, and the gcc12 legs were skipped.
Set the repository variable MERGE_QUEUE_FAST to false to restore the full matrix.

Top-level jobs (14)
Job Result
binaries ⏩ skipped
build_and_test ✅ success
changes ✅ success
config_devdeps ✅ success
config_source_build ⏩ skipped
config_wheeldeps ⏩ skipped
devdeps ✅ success
docker_image ⏩ skipped
gen_code_coverage ⏩ skipped
metadata ✅ success
python_metapackages ⏩ skipped
python_wheels ⏩ skipped
source_build ⏩ skipped
wheeldeps ⏩ skipped
⏩ Skipped jobs (9) — intentionally skipped on PR builds; run on merge_group / workflow_dispatch
Job
binaries
config_source_build
config_wheeldeps
docker_image
gen_code_coverage
python_metapackages
python_wheels
source_build
wheeldeps
All sub-jobs (24) — every matrix leg, with links
Job Status Link
Build and test (amd64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, llvm, openmpi) / Dev environment (Python) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Python) ✅ success view
CI Summary ❔ in_progress view
Check for stable CUDA-Q changes ✅ success view
Configure build (devdeps) ✅ success view
Configure build (source_build) ⏩ skipped view
Configure build (wheeldeps) ⏩ skipped view
Create CUDA Quantum installer ⏩ skipped view
Create Docker images ⏩ skipped view
Create Python metapackages ⏩ skipped view
Create Python wheels ⏩ skipped view
Gen code coverage ⏩ skipped view
Load dependencies (amd64, llvm) / Caching ✅ success view
Load dependencies (amd64, llvm) / Finalize ✅ success view
Load dependencies (amd64, llvm) / Metadata ✅ success view
Load dependencies (arm64, llvm) / Caching ✅ success view
Load dependencies (arm64, llvm) / Finalize ✅ success view
Load dependencies (arm64, llvm) / Metadata ✅ success view
Load source build cache ⏩ skipped view
Load wheel dependencies ⏩ skipped view
Prepare cache clean-up ✅ success view
Retrieve PR info ✅ success view
✅ Required checks (4/4) — declared in .github/required-checks.yml for merge_group_fast
Required check Status Link
Build and test (amd64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, llvm, openmpi) / Dev environment (Python) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Python) ✅ success view

Merged via the queue into NVIDIA:main with commit 1800140 Aug 27, 2026
75 checks passed
@schweitzpgi
schweitzpgi deleted the ch-memtoreg branch August 27, 2026 22:47
github-actions Bot pushed a commit that referenced this pull request Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core compiler python-lang Anything related to the Python CUDA Quantum language implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants