-
Notifications
You must be signed in to change notification settings - Fork 16
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 ExtractCircuitsPass #284
Conversation
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.
clang-tidy made some suggestions
Needs a merge with main. Reviewing now. |
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.
Couple minor things, but mostly LGTM. Also please update the copyright year in all the files you touched.
using namespace mlir; | ||
using namespace mlir::quir; | ||
|
||
namespace { |
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.
LLVM style recommends anonymous namespaces only for class definitions and to use static
for objects and functions. This localizes the information to where in the file it is useful to quickly understand while reading the code and reduces indentation overall generally (though I know you haven't indented here).
currentOp = currentOp->getNextNode()->getNextNode(); | ||
} | ||
|
||
while (currentOp) { |
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 a few comments in the body of this loop would help in understanding the flow.
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.
Added comments with 41febe7.
Also removed special handling of qcs.DelayCycleOp
and verified test case binary output vs without circuits.
Co-authored-by: mbhealy <[email protected]>
Co-authored-by: mbhealy <[email protected]>
Co-authored-by: mbhealy <[email protected]>
Co-authored-by: mbhealy <[email protected]>
Co-authored-by: mbhealy <[email protected]>
Co-authored-by: mbhealy <[email protected]>
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.
clang-tidy made some suggestions
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.
clang-tidy made some suggestions
@mbhealy This should be ready for your re-review. |
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.
LGTM
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.
Generally LGTM, just a few questions. The arguments for extracting circuits after reordering/merging makes sense and I think this is really nice
// control flow found, no next quantum op | ||
return std::nullopt; | ||
} | ||
if (isa<qcs::ParallelControlFlowOp>(nextOp)) |
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.
don't we need a case here for if/else ops as well?
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.
Here we do not as they are covered but the RegionBranchOpInterface
|
||
// if firstQuantumOp is still set then there is an in progress circuit to be | ||
// ended | ||
if (firstQuantumOp) { |
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.
does this condition ever true? because line 281 set firstQuantumOp = nullptr
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.
Yes you are right, I removed with d6b699b.
Co-authored-by: reza-j <[email protected]>
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.
LGTM
This PR adds a new ExtractCircuitsPass. This pass will walk the main function and extract quantum operations into `quir.circuit`s. It is intended to be run after all reordering and merging has been completed. The pass currently requires --enable-circuits=true in order to have effect. Circuit formation during QUIRGen has been deprecated. This form of circuit generation is now controlled by the --enable-circuits-from-qasm command line option. --------- Co-authored-by: mbhealy <[email protected]> Co-authored-by: reza-j <[email protected]>
This PR adds a new ExtractCircuitsPass. This pass will walk the main function and extract quantum operations into `quir.circuit`s. It is intended to be run after all reordering and merging has been completed. The pass currently requires --enable-circuits=true in order to have effect. Circuit formation during QUIRGen has been deprecated. This form of circuit generation is now controlled by the --enable-circuits-from-qasm command line option. --------- Co-authored-by: mbhealy <[email protected]> Co-authored-by: reza-j <[email protected]>
This PR adds a new ExtractCircuitsPass. This pass will walk the main function and extract quantum operations into
quir.circuit
s. It is intended to be run after all reordering and merging has been completed. The pass currently requires--enable-circuits=true
in order to have effect.Circuit formation during QUIRGen has been deprecated. This form of circuit generation is now controlled by the --enable-circuits-from-qasm command line option.