Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Introduce OpenQASM 3 (OQ3) dialect #34

Merged
merged 29 commits into from
Mar 20, 2023
Merged

Introduce OpenQASM 3 (OQ3) dialect #34

merged 29 commits into from
Mar 20, 2023

Conversation

vrpascuzzi
Copy link
Contributor

We introduce the first phase of decoupling our OpenQASM 3 frontend from our QUIR middle-end to define the structure and space for growing our OpenQASM 3 frontend with the means of MLIR.

This is the first phase, migrating only the relevant QUIR Operations to OQ3, leaving QUIR types and attributes as is. Additionally, pattern-matching can be further, more cleanly, separated between QUIR and OQ3, as well as lowering between the two dialects (presently, QUIR and OQ3 go directly to standard dialects).

@vrpascuzzi
QUIR_{AssignArrayElement,DeclareVariable}Op -> OQ3_{...}
adafe65 
QUIR_VariableAssignOp -> OQ3_AssignVariableOp
@vrpascuzzi
QUIR_{UseArrayElementOp,UseVariableOp} -> OQ3_{...}
df3e6b8
@vrpascuzzi
Cbit -> CBit
f8df914
@vrpascuzzi
Migrate CBit ops from QUIR to OQ3 dialect (Part 1)
ea5e1aa 
N.B. `variable_elimination3.mlir` currently failing due to:
```
error: failed to legalize operation 'oq3.assign_cbit_bit' that
was explicitly marked illegal
```
@vrpascuzzi
Remove QUIR CBit remnants
8203f0e
@vrpascuzzi
QUIR_Angle.*Op -> OQ3_Angle.*Op
f57d00f
@vrpascuzzi
QUIR duration ops -> OQ3 duration ops; some small refactoring
2d91bee
@vrpascuzzi
Remove QUIR pattern remnants
0079ddf
@vrpascuzzi
QUIR_CastOp -> OQ3_CastOp
d2186cc
@vrpascuzzi
{QUIR -> OQ3}_DeclareStretchOp
1184ec9
@vrpascuzzi
{QUIR -> OQ3}_{Duration, Stretch}
1f47a54 
Plus some additional clean-up
@vrpascuzzi
Remove QUIR CastOp remnants
5b48309
@vrpascuzzi
QUIR_CallKernel -> OQ3_KernelCall
cea40ea
@vrpascuzzi
Add inlining support for OQ3 dialect
10a1343
@vrpascuzzi
Add missing CBit and variable patterns
bc53cf4
@vrpascuzzi
Use single population function for all conversions (cast+CBit)
998c590
@vrpascuzzi
Fix comments, copyrights; remove unused files
9a81c6e
@vrpascuzzi vrpascuzzi self-assigned this Jan 31, 2023
mhillenbrand
mhillenbrand reviewed Feb 13, 2023
View reviewed changes
Copy link
Contributor

@mhillenbrand mhillenbrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've posted a few comments and questions from my first round of review:

test/Dialect/OQ3/IR/ops.mlir Show resolved Hide resolved
test/Dialect/OQ3/IR/ops.mlir Show resolved Hide resolved
mock_target/Conversion/QUIRToStandard/QUIRToStandard.cpp Show resolved Hide resolved
include/Dialect/OQ3/IR/OQ3VariableOps.td Outdated Show resolved Hide resolved
include/Dialect/OQ3/IR/OQ3VariableOps.td Show resolved Hide resolved
@vrpascuzzi vrpascuzzi mentioned this pull request Feb 17, 2023
Open
taalexander
taalexander requested changes Feb 24, 2023
View reviewed changes
Copy link
Collaborator

@taalexander taalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Vince, mostly LGTM

include/Dialect/OQ3/IR/OQ3Base.td Show resolved Hide resolved
include/Dialect/OQ3/IR/OQ3GateOps.td Outdated Show resolved Hide resolved
include/Dialect/OQ3/IR/OQ3KernelOps.td Outdated Show resolved Hide resolved
@vrpascuzzi
Copy link
Contributor Author

vrpascuzzi commented Mar 1, 2023 •
edited
Loading

How much work would it be to rename this qasm or qasm3 and define this as an MLIR reflection of OpenQASM3 by definition? I think this would make exmaples/code much clearer but it is not strictly necessary.

Are you referring to the renaming oq3 -> qasm(3)?

Is it necessary to separate all of the ops into separate TD files? IMO, there aren't that many. For an example of modern MLIR dialect specifications see https://github.com/llvm/llvm-project/tree/main/mlir/include/mlir/Dialect

I based the separation on modern (as of 14.0.6) MLIR. See, e.g., SPIRV dialect (which was largely the influence behind the design implemented here.)

Can we please add an ops.mlir test for the new dialect (similar to https://github.com/Qiskit/qss-compiler/blob/main/test/Dialect/QUIR/IR/ops.mlir) to test the core dialect functionality (its also a great example file).

For sure. My oversight here.

When updating our internal targets can we please make sure to add this new dialect to the docs (message me if you don't know where to do this)

Will do--thanks!

@taalexander
Copy link
Collaborator

Are you referring to the renaming oq3 -> qasm(3)?

Yes, we can keep as OQ3 as discussed internally.

Is it necessary to separate all of the ops into separate TD files? IMO, there aren't that many. For an example of modern MLIR dialect specifications see https://github.com/llvm/llvm-project/tree/main/mlir/include/mlir/Dialect

I based the separation on modern (as of 14.0.6) MLIR. See, e.g., SPIRV dialect (which was largely the influence behind the design implemented here.)

Thanks for the pointer. This is fine and not a blocker.

@vrpascuzzi vrpascuzzi force-pushed the vrp-iss651-qasm3-mlir-dialect branch from 4f5f3d6 to fb5b14c Compare March 10, 2023 21:28
@vrpascuzzi
Copy link
Contributor Author

Reviews addressed. Only [potentially] outstanding is if we want OQ3 -> QASM/QASM3/OPENQASM3.

@mhillenbrand, @kitbarton, @taalexander, @mbhealy I prefer to take a democratic approach to this. Once decided, I think we are finally ready to LGTM.

mhillenbrand
mhillenbrand approved these changes Mar 13, 2023
View reviewed changes
Copy link
Contributor

@mhillenbrand mhillenbrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm fine with keeping the name of the dialect as it is.

@vrpascuzzi
Copy link
Contributor Author

@taalexander Are we g2g?

@taalexander
Copy link
Collaborator

If the accompanying PR internally is, otherwise we can't merge until that point.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@vrpascuzzi
Copy link
Contributor Author

If the accompanying PR internally is, otherwise we can't merge until that point.

@taalexander Ready.

@vrpascuzzi vrpascuzzi merged commit 37eb723 into main Mar 20, 2023
@taalexander taalexander deleted the vrp-iss651-qasm3-mlir-dialect branch April 5, 2023 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taalexander taalexander taalexander requested changes

@mhillenbrand mhillenbrand mhillenbrand approved these changes

@mbhealy mbhealy Awaiting requested review from mbhealy

Assignees

@vrpascuzzi vrpascuzzi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vrpascuzzi @taalexander @mhillenbrand