-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Migrate init stage to plugins #10689
Conversation
This commit updates the preset pass manager construction to only use plugins for the init stage. To accomplish this the previously hard coded built-in pass manager used for each optimization level are refactored to be in a plugin named "default". One thing that is changed in this PR is that the use of `generate_control_flow_options_check()` is moved to the `pre_init` stage. The reason for this is because the way the init stage was being constructed in the preset pass managers was to do initial checking of any methods, and this was unconditionally being run. This is a more logical fit for pre_init stage because it should run before any specified plugin. Fixes: Qiskit#10687 Fixes: Qiskit#8661
One or more of the the following people are requested to review this:
|
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.
Again, this looks pretty sensible to me.
@@ -98,7 +85,7 @@ def level_0_pass_manager(pass_manager_config: PassManagerConfig) -> StagedPassMa | |||
"scheduling", scheduling_method, pass_manager_config, optimization_level=0 | |||
) | |||
|
|||
init = common.generate_control_flow_options_check( | |||
pre_init = common.generate_control_flow_options_check( |
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 whole concept of a pre-initialisation stage is funny to me, but this does seem like an ok compromise.
init = common.generate_unroll_3q( | ||
pass_manager_config.target, | ||
pass_manager_config.basis_gates, | ||
pass_manager_config.approximation_degree, | ||
pass_manager_config.unitary_synthesis_method, | ||
pass_manager_config.unitary_synthesis_plugin_config, | ||
pass_manager_config.hls_config, | ||
) |
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.
Pre-existing, but is this definitely an "initialisation" action and not just a shared requirement of most of our routing plugins?
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.
It is, since almost all layout and routing stages only work with 1 and 2 qubit gates this is why it's in the default. You could argue it's a better fit for the pre_layout
stage, but I think init
works better here because it allows for plugins to override it. So in a hypothetical case where there is a layout and routing pass for hardware that has native 3q gates they can have users do something like transpile(..., routing_method="toffoli", init_method="only_opt")
.
I guess there is an argument that the init
stage should be more granular. We do have an issue for adding a dedicated synthesis stage #8936. But, at the end of the day there isn't very much in there and I worry about having too many stages that all only do one or two passes.
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.
Yeah, that's fine. I'd originally written "layout" in my comment, then mistakenly changed it because I thought it was only Sabre that really had a problem (which is via Sabre routing anyway - the Sabre layout logic itself doesn't care), but that's not true - noise aware, CSP and VF2 all have issues with 3q.
This commit updates the preset pass manager construction to only use plugins for the init stage. To accomplish this the previously hard coded built-in pass manager used for each optimization level are refactored to be in a plugin named "default". One thing that is changed in this PR is that the use of `generate_control_flow_options_check()` is moved to the `pre_init` stage. The reason for this is because the way the init stage was being constructed in the preset pass managers was to do initial checking of any methods, and this was unconditionally being run. This is a more logical fit for pre_init stage because it should run before any specified plugin. Fixes: Qiskit#10687 Fixes: Qiskit#8661
Summary
This commit updates the preset pass manager construction to only use plugins for the init stage. To accomplish this the previously hard coded built-in pass manager used for each optimization level are refactored to be in a plugin named "default". One thing that is changed in this PR is that the use of
generate_control_flow_options_check()
is moved to thepre_init
stage. The reason for this is because the way the init stage was being constructed in the preset pass managers was to do initial checking of any methods, and this was unconditionally being run. This is a more logical fit for pre_init stage because it should run before any specified plugin.Details and comments
Fixes: #10687
Fixes: #8661