[QIR] Insert array record call in sampling workflow #3553
Conversation
inset an array recording output call pripr to any record result calls. This pass should be used in `sample` workflow. Signed-off-by: Pradnya Khalate <[email protected]>
|
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
|
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
|
|
||
| LLVM_DEBUG(llvm::dbgs() << "Before adding array recording call:\n" | ||
| << *funcOp); | ||
| if (failed(insertArrayRecordingCalls(funcOp, analysis.arraySize, |
There was a problem hiding this comment.
Should use the intrinsic loading instead of rolling your own. Aren't these already loaded by the QIR API Prep pass, though?
There was a problem hiding this comment.
I wanted to make this pass self-contained, since we cannot guarantee that the prep pass has run before this one.
Should I move the loadIntrinsic call from line#176 here?
There was a problem hiding this comment.
Hmm.
Well, the QIR lowering prep(aration) pass should, by definition, always run prior to lowering things to QIR. If we're generating QIR code/calls and don't know if the prep pass was run, we're doing something quite wrong.
Signed-off-by: Pradnya Khalate <[email protected]>
khalatepradnya
force-pushed
the
insert-array-record
branch
from
ff1bb2a to
c774568
Compare
|
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
* Update label to be of type `i1` instead of `i8` since we interpret the results as Booleans. * Test for parser. Signed-off-by: Pradnya Khalate <[email protected]>
|
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
Signed-off-by: Pradnya Khalate <[email protected]>
|
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
|
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
|
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
| // if the QIR produced by a sampled kernel emits a sequence of RESULT | ||
| // records without enclosing them in an ARRAY, we interpret them | ||
| // collectively as an array of results. | ||
| // NOTE: This assumption prevents us from correctly supporting `run` with |
There was a problem hiding this comment.
What is needed to get rid of this workaround?
There was a problem hiding this comment.
I think the proper way / time would be when we disallow user-specified measurements in sampled kernels, then we can also drop the index from label logic on lines 144-172.
|
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
|
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
|
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
| // measurement operations. When there are no explicit stores, track the first | ||
| // measurement operation and the get the total number of measurements. | ||
| struct AllocaMeasureStoreAnalysis { | ||
| AllocaMeasureStoreAnalysis() = default; |
There was a problem hiding this comment.
Why is this default ctor needed? Shouldn't the analysis always be run?
| // Walk a function to identify all the measure-discriminate-store patterns and | ||
| // collect the associated `AllocaOp` when the measurement results are stored. | ||
| // Collect only unique AllocaOps - since each may correspond to multiple | ||
| // measurement operations. When there are no explicit stores, track the first | ||
| // measurement operation and the get the total number of measurements. |
There was a problem hiding this comment.
What does "unique AllocaOps" mean? What happens if the variable (the alloca slot) is used by other loads and stores, which may have nothing to do with any quantum operations? What happens if the location being stored to is not an AllocaOp? How do you count measurements? A measurement can measure multiple qubits.
|
|
||
| // Find the operands derived from measurements | ||
| for (auto operand : op->getOperands()) { | ||
| if (valueToMeasurement.count(operand)) { |
There was a problem hiding this comment.
This doesn't look correct. It should be ok if the SSA value is lexically subsequent to the defining op, but that's not going to always be true in legal IR. Effectively, it does not consider the control flow issues.
I've said this in other PRs, this sort of roll-your-own pattern matching in each pass is going to become a maintenance obstacle as we go along. We really need to be using the factored use-def (value semantics) form of the IR to our advantage for these rewrites.
| conversion passes, then inserts array recording instruction based on the analysis. | ||
| }]; | ||
| let dependentDialects = ["quake::QuakeDialect", "cudaq::cc::CCDialect", | ||
| "mlir::LLVM::LLVMDialect"]; |
There was a problem hiding this comment.
I don't see where the LLVMDialect is used. Did I miss it?
I think it would be ok to delete this dependentDialects since we know the code will have Quake and CC dialects in the input. (We don't need to check that those dialects were loaded.)
3 participants
This PR introduces a new module pass,
QirInsertArrayRecord, that analyzes instruction patterns and inserts array recording instruction required for proper measurement result handling in QIR pipelines. The pass identifies measurement storage patterns and ensures the QIR output declares the correct array size and type label for measurement results.This PR also enables the pass in compilation workflow.