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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dialects: Add qssa dialect #2746
dialects: Add qssa dialect #2746
Changes from 6 commits
eff3ab8
289ad3f
931c964
860d0fa
b012842
3fb88e9
ba21bca
625227c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
num
,count
,num_qubits
or similar might be better for inferring the type from a point of readability, since the definitionqubit = QubitAttr()
would suggest thatqubits : Sequence[QubitAttr]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, looking at this again, I don't think we need this attribute at all. The number of qubits is simply the number of results. So the attribute can be dropped entirely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I'd generally encourage to print some type information in the mlir, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the semantics, this reads like it allocates a series of single qubits, rather than an array of qubits as the op is described in the paper. You could consider parameterising
QubitAttr
similar toIntegerAttr
, or maybe @AntonLydike could advise?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I was saying is that these two ops are both completely valid, just one has redundant information:
and
Simply because we know that
qbits == len(res)
. Therefore we don't need to carry this additional information at all. The resulting IR would look like this:So you just specify how many qbits you allocate by the number of results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference: 3.3.1 Allocation.
qssa.alloc
allocates an array of qubits of fixed or variable length.%q = qssa.alloc : qubit<10>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @alexarice does not talk about qbit arrays, but models each qbit as a single SSA value. But that's for him to answer 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seem to remember that semantically, an array of qubits may be understood to mean something different as compared to n single qubits, but I may be mistaken here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not directly based on the qssa dialect from the paper and is currently representing each individual qubit as a separate ssa value. I'd be open to renaming this dialect if this will cause confusion in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the discussion didn't load for me on github, but I believe dropping the parameter is not ideal as the parser does not have access to the number of results (at least from my testing)