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.

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

Add quir.declare_parameter, quir.load_parameter and generate quir.circuits #100

Merged
merged 35 commits into from
May 2, 2023

Conversation

bcdonovan
Copy link
Contributor

This PR adds two new operations to the QCS dialect, declare_parameter and parameter_load. These operations are used to support input parameters, specifically those defined in QASM as

input type name = value;

The PR also updates the QUIRGenQASM3Visitor to handle input parameters as well as put real time operations into quir.circuits.

A command line option --enable-parameters is used as a feature flag to gate use of the new features.

New tests have been added to check the new features. All existing tests should pass without the new feature flag. Enabling support for the new feature throughout the stack and removing the feature flag is reserved for a future PR.

This PR does not change the handling of gate functions. That is quir.circuits are not inserted into the functions generated by QASM3 gate h q { } statements.

@github-actions
Copy link
Contributor

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

Copy link
Contributor

@mbhealy mbhealy left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just a couple comments here and there.

lib/Frontend/OpenQASM3/QUIRGenQASM3Visitor.cpp Outdated Show resolved Hide resolved
lib/Frontend/OpenQASM3/QUIRGenQASM3Visitor.cpp Outdated Show resolved Hide resolved
lib/Frontend/OpenQASM3/QUIRGenQASM3Visitor.cpp Outdated Show resolved Hide resolved
lib/Frontend/OpenQASM3/QUIRGenQASM3Visitor.cpp Outdated Show resolved Hide resolved
test/Frontend/OpenQASM3/input-parameters-if.qasm Outdated Show resolved Hide resolved
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.

Overall this looks very good to me, and much further along than I was expecting 😄 🎉.

include/Dialect/QCS/IR/QCSOps.td Outdated Show resolved Hide resolved
include/Dialect/QCS/IR/QCSOps.td Outdated Show resolved Hide resolved
include/Dialect/QCS/IR/QCSOps.td Show resolved Hide resolved
include/Dialect/QCS/IR/QCSOps.td Outdated Show resolved Hide resolved
include/Frontend/OpenQASM3/QUIRVariableBuilder.h Outdated Show resolved Hide resolved
lib/Frontend/OpenQASM3/OpenQASM3Frontend.cpp Outdated Show resolved Hide resolved
lib/Frontend/OpenQASM3/QUIRGenQASM3Visitor.cpp Outdated Show resolved Hide resolved
@@ -238,6 +248,8 @@ QUIRGenQASM3Visitor::reportError(ASTBase const *location,
}

void QUIRGenQASM3Visitor::visit(const ASTForStatementNode *node) {
switchCircuit(false, getLocation(node));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been trying to think of a good way of avoiding the need to repeat switchCircuit in most methods as this could be brittle. I haven't thought of one yet.

@github-actions
Copy link
Contributor

github-actions bot commented May 1, 2023

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

Co-authored-by: Thomas Alexander <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented May 1, 2023

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

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented May 1, 2023

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

@github-actions
Copy link
Contributor

github-actions bot commented May 1, 2023

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

@github-actions
Copy link
Contributor

github-actions bot commented May 1, 2023

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

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.

LGTM, thank you for your changes!

@bcdonovan bcdonovan merged commit 128c937 into main May 2, 2023
@bcdonovan bcdonovan deleted the bd-parameters branch May 2, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants