[Python] Qvector init from state #1713
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…A/cuda-quantum into qvector-init-from-state2
…A/cuda-quantum into qvector-init-from-state2
…A/cuda-quantum into qvector-init-from-state2
…A/cuda-quantum into qvector-init-from-state2
…A/cuda-quantum into qvector-init-from-state2
…A/cuda-quantum into qvector-init-from-state2
…A/cuda-quantum into qvector-init-from-state-update
18 tasks
annagrin
changed the title
annagrin
requested review from
amccaskey,
schweitzpgi,
1tnguyen and
anthony-santana
annagrin
changed the title
annagrin
commented
May 24, 2024
annagrin
merged commit b5ce9a7
into
NVIDIA:experimental/stateHandling
112 checks passed
schweitzpgi
reviewed
May 29, 2024
Comment on lines
+123
to
+125
| auto *ctx = eleTy.getContext(); | ||
| auto eTy = cudaq::opt::factory::getCharType(ctx); | ||
| return cudaq::opt::factory::getPointerType(eTy); |
There was a problem hiding this comment.
Can be simplified to just:
return cudaq::opt::factory::getPointerType(eleTy.getContext());On the other hand, we don't use the eleTy, so we could erase this function and just use getPointerType at the call site.
schweitzpgi
reviewed
May 29, 2024
Comment on lines
+2036
to
+2038
| if (isa<NoneType>(eleTy)) | ||
| return factory::getPointerType(type.getContext()); | ||
|
|
There was a problem hiding this comment.
Why doesn't the recursion handle this? The recursive call on line 2035 ought to handle pointers to pointers.
schweitzpgi
reviewed
May 29, 2024
| @@ -434,8 +434,10 @@ class GenerateKernelExecution | |||
| hasTrailingData = true; | |||
| continue; | |||
| } | |||
| if (isa<cudaq::cc::PointerType>(currEleTy)) | |||
| if (isa<cudaq::cc::PointerType>(currEleTy) && | |||
| !isStatePointerType(currEleTy)) { | |||
There was a problem hiding this comment.
nit: don't need braces.
schweitzpgi
reviewed
May 29, 2024
| @@ -933,6 +935,13 @@ class GenerateKernelExecution | |||
| builder.create<cudaq::cc::StoreOp>(loc, endPtr, sret2); | |||
| } | |||
|
|
|||
| static bool isStatePointerType(mlir::Type ty) { | |||
| if (auto ptrTy = dyn_cast<cudaq::cc::PointerType>(ty)) { | |||
Comment on lines
+953
to
+954
| return isa<cudaq::cc::PointerType>(i.getType()) && | ||
| !isStatePointerType(i.getType()); |
There was a problem hiding this comment.
I don't get this one. We're dropping hidden arguments. Why would a cudaq::state* appear as a this or sret? Do we have an example?
| @@ -1207,8 +1217,9 @@ class GenerateKernelExecution | |||
| hasTrailingData = true; | |||
| continue; | |||
| } | |||
| if (isa<cudaq::cc::PointerType>(inTy)) | |||
| if (isa<cudaq::cc::PointerType>(inTy) && !isStatePointerType(inTy)) | |||
There was a problem hiding this comment.
This doesn't look correct. We cannot pass a cudaq::state* as a pointer-free value.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add initialization of
qvectorfromcudaq::StateDetails
The changes that allow using
cudaq.Statein python kernel functions:Add
cc.StateTypeto python bindingsAllow arguments of StateType to be passed to kernel functions
Made
cudaq.from_statefail on incorrect precision dataUpdate
ast_bridge.pyto create MLIR for initializingqvectorfrom stateSupport capturing states from python non-kernel code
qallocinkernel_builder.pyConvert StateType to
ptr<i8>incudaq::opt::initializeTypeConversionsUpdate
QmemRAIIOpRewriteto call a newextern CAPI on state pointers__quantum__rt__qubit_allocate_array_with_cudaq_state_ptrImplement
__quantum__rt__qubit_allocate_array_with_cudaq_state_ptrstate_helperto access private data)Update related intrinsics
__nvqpp_cudaq_state_numberOfQubits__nvqpp_cudaq_state_vectorData(to avoid a copy in MLIR)Add tests
cudaq.from_statefails on incorrect precision dataSee python usage examples in [RFC] [Language] Quantum allocation with state initialization #1086
Towards: