Repository navigation
Conversation
Signed-off-by: Adam Geller <adgeller@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 |
documentation |
✅ success |
gen_code_coverage |
⏩ skipped |
metadata |
✅ success |
python_devel_wheel |
⏩ skipped |
python_metapackages |
⏩ skipped |
python_wheels |
⏩ skipped |
source_build |
⏩ skipped |
wheeldeps |
✅ success |
⏩ Skipped jobs (8) — intentionally skipped on PR builds; run on merge_group / workflow_dispatch
| Job |
|---|
binaries |
config_source_build |
docker_image |
gen_code_coverage |
python_devel_wheel |
python_metapackages |
python_wheels |
source_build |
All sub-jobs (45) — 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 |
| Create shared Python development wheel | ⏩ skipped | view |
| Documentation / Build and check docs | ✅ success | 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 (7/7) — 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 |
| Documentation / Build and check docs | ✅ success | view |
Signed-off-by: Adam Geller <adgeller@nvidia.com>
Signed-off-by: Adam Geller <adgeller@nvidia.com>
Signed-off-by: Adam Geller <adgeller@nvidia.com>
…match allocation order when deallocs are present. Signed-off-by: Adam Geller <adgeller@nvidia.com>
Signed-off-by: Adam Geller <adgeller@nvidia.com>
| if (useEvince && analysis.hasEvinces()) { | ||
| if (failed(addMeasurementsBefore(func, analysis.evinces))) | ||
| signalPassFailure(); |
There was a problem hiding this comment.
The following repro.cpp
#include <cudaq.h>
struct test {
void operator()(bool b) __qpu__ {
cudaq::qubit q;
x(q);
if (b) {
cudaq::qubit a;
x(a);
}
}
};
int main() {
cudaq::sample(10, test{}, true).dump();
}
shows output as
{ 1:10 }
Shouldn't it be { 11:10 }? I think in evince mode, we should also measure in front of the deallocations of qubits that are not covered by an evince (for example, nested allocations)? Or the other option is to have inject-implicit-output create evinces for nested allocations too.
There was a problem hiding this comment.
We don't really want to spend time beefing up automatic injection of evince operations. The entire pass is a hack to work around other more serious problems.
Which is to say, beefing up that pass is a waste of time as the pass will be removed entirely very soon. Which means, this approach for adding the measurements is a temporary band-aid. Which is to say, the insertion of phantom measurements is either wrong or misguided even though it is papering over gaps in other passes.
There was a problem hiding this comment.
This is a short-term bandaid to try to reduce the amount of implicit behavior. In this case using the add-measurements pass instead of inserting the measurements in the mapper, so it's at least an explicit pass. @schweitzpgi is working on a longer term solution to remove the need for implicit behavior.
| // The caller owns the qubits, so drop deallocations of the argument. | ||
| initFunc.walk([&](cudaq::quake::DeallocOp dealloc) { | ||
| if (isa<BlockArgument>(traceToAllocation(dealloc.getReference()))) | ||
| dealloc.erase(); | ||
| }); | ||
|
|
||
| // The caller is also responsible for making its own qubits observable. Drop | ||
| // the compiler-generated evince ops on the argument, which would otherwise | ||
| // be observation points in the middle of the caller's circuit. | ||
| initFunc.walk([&](cudaq::quake::EvinceOp evince) { | ||
| if (evince.getCompilerGenerated() && | ||
| llvm::all_of(evince.getArgs(), [&](Value v) { | ||
| return isa<BlockArgument>(traceToAllocation(v)); | ||
| })) | ||
| evince.erase(); | ||
| }); |
There was a problem hiding this comment.
I don't see any test for the new evince and dealloc removal in state kernel init functions. Let's add a regression case or an end-to-end sample test with a get_state / state-from-kernel argument on an emulated remote target?
| module { | ||
| func.func @reference() attributes {"cudaq-entrypoint", "cudaq-kernel", no_this} { | ||
| %0 = quake.alloca !quake.veq<2> | ||
| %1 = quake.alloca !quake.ref | ||
| %2 = quake.extract_ref %0[0] : (!quake.veq<2>) -> !quake.ref | ||
| quake.h %2 : (!quake.ref) -> () | ||
| quake.x %1 : (!quake.ref) -> () | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Ah seems like it does not cover nested allocations or early returns in evince mode. Should we add tests for an allocation made inside cc.if or cc.scope? And multiple returns?
There was a problem hiding this comment.
It would be better to fall back to using deallocations than to try to build the house of cards around evince even higher. (The entire purpose of evince is not to be a workaround for removing, adding, or teleporting measurement operations around in the IR.)
| let description = [{ | ||
| This pass adds measurement operations to the kernels that have no | ||
| measurements. | ||
|
|
||
| This pass should be run in the sampling context, so as to comply with the | ||
| spec - If no measurements appear in the kernel, then the `__global__` | ||
| register is formed with implicit measurements being added for all the | ||
| qubits defined in the kernel, and the measurements all occur at the end of | ||
| the kernel. | ||
|
|
There was a problem hiding this comment.
Let's add a short paragraph describing the new mode use-evince? Also, when the dealloc or return paths are used instead.
schweitzpgi
left a comment
There was a problem hiding this comment.
+1 Thanks for grinding through this, Adam.
Motivation
Mapping.cppinserted implicit measurements for unmeasured qubits as a side effect of mapping. This is unpleasant for several reasons:add-measurements' job, having duplicate functionality is never good.This PR removes these implicit measurements, making
add-measurementsthe single place where implicit sampling measurements are added.Changes
Mapping.cpp: drop the implicit-measurement insertion. The mapping lit tests now carry explicit measurements of their own.Compiler.cpp: move the sample-onlyadd-measurementscall to beforeexecuteMainPipeline. With the mapper no longer measuring, running it afterwards measured physical qubits and emitted an emptymapping_reorder_idx, so sample results came back in physical rather than program order ('01'for'10'). This affects every target whose pipeline hasqubit-mappingbut noadd-measurementsof its own: oqc, iqm, anyon.add-measurements: newuse-evinceoption (defaultfalse).add-deallocsdoes not insert deallocations in a stable order, whereas implicit measurements are assumed to be added in allocation order.inject-implicit-outputemitsquake.evinces in allocation order, which is preserved, providing a workaround. It should be noted that using this new option as the default across the codebase causes no issues, and may be a good future direction to take the pass to avoid ordering issues.ArgumentConversion.cpp:createInitFuncreplaces the callee'sallocawith a subveq of the caller's veq, so inlined state-producing kernels leave behind deallocations and compiler-generated evince ops that act on qubits the caller owns. Both are now dropped via a sharedtraceToAllocationhelper; otherwise they become deallocations and observation points in the middle of the caller's circuit.Behavior change
Measurements are now added before the main pipeline runs, so ancilla qubits introduced by multicontrol decomposition are no longer measured and no longer appear in sample bitstrings. Affected expectations in
test_kernel_builder.pyandQuantinuumTester.cppare updated.