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

Replaces the uses of quir call with the converted pulse call in QuirToPulse #273

Merged
merged 3 commits into from
Mar 7, 2024
Merged

Replaces the uses of quir call with the converted pulse call in QuirToPulse #273

merged 3 commits into from
Mar 7, 2024

Conversation

reza-j
Copy link
Contributor

@reza-j reza-j commented Feb 26, 2024

This PR replaces all the uses of quir circuits with their corresponding pulse call, which is needed for dynamic circuits when we want to make a decision based on the measurement result.

The PR also copies over the quir attributes of measure op to their corresponding pulse call.

@reza-j reza-j added the no-reno Disable checking for a releasenote label Feb 26, 2024
@reza-j reza-j requested a review from a team as a code owner February 26, 2024 18:55
ModuleOp moduleOp);
mlir::pulse::CallSequenceOp
convertCircuitToSequence(mlir::quir::CallCircuitOp callCircuitOp,
mlir::func::FuncOp &mainFunc, ModuleOp moduleOp);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is one thing that confuses me about MLIR operations. The callCircuitOp and moduleOp are not explicitly passed by reference, but the mainFunc is.
Do you know if these are still being passed by reference, or if this is going to pass by value? Pass by value will create copies, which is inefficient. It is better to pass by reference and mark as const to indicate they will not be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this today. There might be a better way to do this, but what I did was trying to print the memory location of variables before calling the function and inside the function with:

std::cout << &callCircuitOp << "\n"
std::cout << &moduleOp << "\n"

case1: function arguments are (mlir::quir::CallCircuitOp callCircuitOp, mlir::func::FuncOp &mainFunc, ModuleOp moduleOp)

case2: function arguments are (mlir::quir::CallCircuitOp &callCircuitOp, mlir::func::FuncOp &mainFunc, ModuleOp &moduleOp)

I noticed that in case1, the memory addresses were different before calling the function and inside the function, so my guess is that it copies in that case. While for case2, memory addressed were the same

given this, I added changes to have & in function args

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if this is one of the cases where we can use const. I tried e.g., const mlir::quir::CallCircuitOp &callCircuitOp and got a compile error saying

error: 'this' argument to member function 'getOperand' has type 'const mlir::quir::CallCircuitOp', 
but function is not marked const
          auto durOpConstantOp = callCircuitOp.getOperand(argNum)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I see. MLIR seems to be missing a lot of const versions of the methods. I can see this would be painful to try and figure out what can be const as a result.
I think this is ok, we just need to be mindful that any changes that are made to the op now will persist (there shouldn't be any, but need to keep it in mind).

@reza-j reza-j requested a review from kitbarton March 6, 2024 18:09
Copy link
Collaborator

@kitbarton kitbarton 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 changing the parameters to pass-by-reference.

@reza-j reza-j merged commit 414d78e into openqasm:main Mar 7, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-reno Disable checking for a releasenote
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants