Add option to control unwind lowering - #5274
Conversation
Signed-off-by: Thien Nguyen <thiennguyen@nvidia.com>
Signed-off-by: Thien Nguyen <thiennguyen@nvidia.com>
CI Summary (
|
| 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 |
schweitzpgi
left a comment
There was a problem hiding this comment.
Looking good. I think we should disentangle unwinding and inlining. I don't know why they are conflated in the pipeline today. Weirdness.
| /// to kernels remain in the fully inlined into entry point kernel. | ||
| void addAggressiveInlining(mlir::OpPassManager &pm, bool fatalCheck = false); | ||
| void addAggressiveInlining(mlir::OpPassManager &pm, bool fatalCheck = false, | ||
| bool lowerUnwind = true); |
There was a problem hiding this comment.
These are separate passes that ought to have no relationship to each other. I'm not sure why unwinding was buried in the inlining pipeline. It should be pulled out and stand on its own feet at this point.
| void cudaq::opt::addAggressiveInlining(OpPassManager &pm, bool fatalChecks, | ||
| bool lowerUnwind) { | ||
| llvm::StringMap<OpPassManager> opPipelines; | ||
| pm.addNestedPass<func::FuncOp>(cudaq::opt::createUnwindLowering()); |
There was a problem hiding this comment.
Yeah, this is truly weird. Unwinding ought to happen very early. Once done, it should never be thought about again. It's a pass for lowering very high-level syntactic goop from the front-end to a correct and selectable lower-level IR. That selectability is precisely what we want now.
| // complicated case. | ||
| // - If the dfJump is continue, then we want to allow the step region (if | ||
| // any) to execute. We also clear the dfJump flag if the while region. | ||
| if (loop.hasStep()) { |
There was a problem hiding this comment.
Tried running the following code
import cudaq
cudaq.set_target("quake_fake")
@cudaq.kernel
def test(n: int) -> int:
total = 0
for i in range(3):
if i == n:
break
total += 1
return total
cudaq.run(test, 1, shots_count=1)
it fails with an error as
error: operand #0 does not dominate this use
Traceback (most recent call last):
File "/usr/local/cudaq/cudaq/kernel/ast_bridge.py", line 6273, in compile_to_mlir
cudaq_runtime.runPassManager(pm, bridge.module)
RuntimeError: pass pipeline failed
Seems like when a cc.loop has a step region, the pass moves the step region's body into a new guard cc.if but leaves the region's terminator behind. %4 is now defined inside the if, so the IR fails the dominance verifier. The while-region case right below handles exactly this by yielding the value out of the guard. The step region case does not.
| def early_return(return_early: bool) -> int: | ||
| i = 0 | ||
| while i < 4: | ||
| if return_early: | ||
| return 1 | ||
| i += 1 | ||
| return i | ||
|
|
||
|
|
||
| @cudaq.kernel | ||
| def branch_returns(first_branch: bool) -> int: | ||
| if first_branch: | ||
| return 7 | ||
| else: | ||
| return 9 | ||
|
|
||
|
|
||
| @cudaq.kernel | ||
| def loop_control(skip: int, stop: int) -> int: | ||
| i = 0 | ||
| total = 0 | ||
| while i < 6: | ||
| if i == stop: | ||
| break | ||
| if i == skip: | ||
| i += 1 | ||
| continue | ||
| total += i | ||
| i += 1 | ||
| return total |
There was a problem hiding this comment.
Let's add for loop variants with break, continue and early return as well?
for i in range(3):
if i == n:
break
There was a problem hiding this comment.
Yeah, this is an edge case that we're investigating. I'll add the test case when it is fixed.
Signed-off-by: Thien Nguyen <thiennguyen@nvidia.com>
…nd add tests Signed-off-by: Thien Nguyen <thiennguyen@nvidia.com>
Signed-off-by: Thien Nguyen <thiennguyen@nvidia.com>
Signed-off-by: Thien Nguyen <thiennguyen@nvidia.com> # Conflicts: # python/cudaq/kernel/kernel_decorator.py
Signed-off-by: Thien Nguyen <thiennguyen@nvidia.com>
Added configurable AOT unwind-mode: cfg (default), dataflow, or none.
Wired the option through target YAML, C++
nvq++, and Python compilation pipelines.Updated aggressive inlining pipeline which also runs unwind lowering
Initialized the dataflow pass’s
dfJumpstate (None) to prevent undefined value.Added C++/Python fake-target and focused MLIR regression coverage.