Rejecting reading a for loop variable after a zero-trip loop

[sacpis]

In Python a for loop that never runs leaves its loop variable unbound, so reading it afterwards raises NameError. In a kernel the read silently returned whatever was in the variable's stack slot:

@cudaq.kernel
def k(n: int):
    q = cudaq.qvector(3)
    for i in range(n):
        h(q[i])
    x(q[i])

cudaq.sample(k, 0) 

• Output before

# RuntimeError: Provided index [18446744073709551615] >= array size [3]

The bridge gives i a slot in the function entry block but never writes it before the loop, so a zero-trip loop leaves it uninitialized.

A kernel has no way to raise NameError at runtime, so the bridge now rejects the read at compile time when it cannot prove the loop runs at least once.

• Output after

cudaq.kernel.ast_bridge.CompilerError: test.py:6: error: loop variable(s) i may be read (line(s) 8) after a loop that can run zero times, which leaves them unbound in Python - assign them before the loop, or use a loop whose trip count is known to be non-zero
         (offending source -> for i in range(n):
    h(q[i]))

Fixes #5250

[github-actions]

CI Summary (push) — ✅ passed

Run #33093354917 · ✅ 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

[1tnguyen]

Overall, LGTM 👍

I just have a couple of comments for some edge cases.

[schweitzpgi]

In the example given, the Python bridge cannot prove anything. It doesn't have enough information. Whether the kernel is correct depends or throws an exception is 100% dependent upon an uncertain runtime value.

Secondly, this is not a loop phenomena. It is a python thing. Consider:

@cudaq.kernel
def dynamic_throw(flag: bool, n: int):
    q = cudaq.qvector(n)
    if flag:
        i = ...
    x(q[i])

This is the same problem, but there is no loop. Actually, this one has multiple potential problems. If $n \le 0$, the qvector allocation has no semantics. Hence any quantum gate applied to q has no semantics. On top of that, i may or may not be defined, meaning the access into the q, which itself may or may not be defined, may or may not have any semantics.

Hopefully, you can see where this slippery slope is heading rather quickly. The options here are:

• allow this kernel and catch the meaningless instantiations at runtime when JIT compilation specializes this and can prove whether the particular activation is semantically sound or not
• statically be paranoid and never allow such a kernel in the first place. (yes, that means not allowing the user to specify the size of a qvector from an argument!)
• the python bridge to take the "middle ground" and construct checks as part of the IR. so, for my example, the Python bridge would generate a check to see if $n \le 0$ and only execute the rest of the kernel if that condition was False. Similarly it would have to check that i was defined and an integer in range...

Currently, we have taken the first bullet approach. I think there is merit to considering a hybrid approach with Python.

I don't think this is a quick fix or something that needs to merge immediately before a code freeze. The third option is clearly extensive and not what this set of diffs provides.

I'm going to request changes for now. We can discuss further of course.

[schweitzpgi]

What would be a good change IMO, would be to add checks to the verifier in code generation that the access to the qvector is well-defined. I'm assuming that is not done, which was the inspiration for this patch?

[sacpis]

In the example given, the Python bridge cannot prove anything. It doesn't have enough information. Whether the kernel is correct depends or throws an exception is 100% dependent upon an uncertain runtime value.

Secondly, this is not a loop phenomena. It is a python thing. Consider:

@cudaq.kernel
def dynamic_throw(flag: bool, n: int):
    q = cudaq.qvector(n)
    if flag:
        i = ...
    x(q[i])

This is the same problem, but there is no loop. Actually, this one has multiple potential problems. If n ≤ 0 , the qvector allocation has no semantics. Hence any quantum gate applied to q has no semantics. On top of that, i may or may not be defined, meaning the access into the q, which itself may or may not be defined, may or may not have any semantics.

Hopefully, you can see where this slippery slope is heading rather quickly. The options here are:

• allow this kernel and catch the meaningless instantiations at runtime when JIT compilation specializes this and can prove whether the particular activation is semantically sound or not
• statically be paranoid and never allow such a kernel in the first place. (yes, that means not allowing the user to specify the size of a qvector from an argument!)
• the python bridge to take the "middle ground" and construct checks as part of the IR. so, for my example, the Python bridge would generate a check to see if
  n
  ≤
  0
  and only execute the rest of the kernel if that condition was False. Similarly it would have to check that i was defined and an integer in range...

Currently, we have taken the first bullet approach. I think there is merit to considering a hybrid approach with Python.

I don't think this is a quick fix or something that needs to merge immediately before a code freeze. The third option is clearly extensive and not what this set of diffs provides.

I'm going to request changes for now. We can discuss further of course.

Yes, you are right on this. What the patch actually rejects is narrower than unprovable. There is no runtime value of n that makes it well defined (if the loop runs, the read is fine, if it does not, plain Python raises NameError). So it is not the same as sizing a qvector from an argument, where the kernel is perfectly good for n > 0. That is the line I tried to draw, but I agree it is a small slice of the general problem and it is drawn in the bridge rather than in the compiler.

Wit respect to the verifier suggestion, quake.extract_ref does already verify a constant index against a statically sized veq. It does not catch this case because the bad value is not an out-of-range constant (it is a load from an
uninitialized stack slot, so the index is opaque and the read silently returns garbage). Tightening the verifier is worth doing on its own, but it will not see this one.

I am thinking of one the these options

1. Hold this PR and fold it into the broader may be unbound / hybrid runtime check design (your option 3). Nothing here needs to land before the freeze.
2. Land only the two nit fixes above (the USub folding is a real, separate bug) and drop the diagnostic.
3. Turn the hard error into a warning for now.

Preferring option 2 for now and then 1 later.

Happy to discuss further.