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

Decomposition updates for quake.control types #2186

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bmhowe23
Copy link
Collaborator

@bmhowe23 bmhowe23 commented Sep 3, 2024

This is a follow-up to #2145 to handle decompositions with more scenarios with quake.control types.

The only scenarios remaining after this PR are threading modified controls through to arbitrary locations. That will need to handle CFG and structured control flow (with arbitrary nesting levels), so I decided to mark that as TODO for now since there is currently no pressing need for that. (The current PR will gracefully not perform transformations if it encounters those more complicated scenarios.)

Copy link

github-actions bot commented Sep 3, 2024

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Sep 3, 2024
@@ -82,6 +85,37 @@ class QuakeOperatorCreator {
quake::WireType::get(rewriter.getContext()));
}

/// @brief Promote all `quake.control` types in \p controls to `quake.wire`
/// types.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This made me wonder if we don't just want to insist that the linear-ctrl-form pass isn't run first?

It seems like decomposition could be run in pruned control form though.

   %42 = quake.foo [%ctrl] %target : (!quake.control, !quake.target) -> !quake.target

might need to expand to

   %a42 = quake.bar %target : (!quake.target) -> !quake.target
   %b42 = quake.baz [%ctrl] %a42 : (!quake.control, !quake.target) -> !quake.target
   %c42 = quake.qux %b42 : (!quake.target) -> !quake.target
   %d42 = quake.quux [%ctrl] %c42 : (!quake.control, !quake.target) -> !quake.target
   %42 = quake.corge %d42 : (!quake.target) -> !quake.target

Some expansion patterns may require the %ctrl qubit to "magically" transform to a target qubit. In those cases, we'd have to add from_ctrl and to_ctrl "inside" the pattern.

   %a42 = quake.bar %target : (!quake.target) -> !quake.target
   %b42 = quake.baz [%ctrl] %a42 : (!quake.control, !quake.target) -> !quake.target
   %from = quake.from_ctrl %ctrl : (!quake.control) -> !quake.target
   %c42:2 = quake.qux %from, %b42 : (!quake.target, !quake.target) -> !quake.target
   %to = quake.to_ctrl %c42#0 : (!quake.target) -> !quake.control
   // Replace the next use of %ctrl (and all uses dominated by the above to_ctrl) with %to
   %d42 = quake.quux [%to] %c42#1 : (!quake.control, !quake.target) -> !quake.target
   %42 = quake.corge %d42 : (!quake.target) -> !quake.target

I guess the real question though is whether this effort is worth it or whether we should just run linear-ctrl-form.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Add: this comment

   // Replace the next use of %ctrl (and all uses dominated by the above to_ctrl) with %to

would mean that a value of type !quake.control that is already passed via a terminator to a block argument would require no additional changes.

The sticky case is if the value is defined in another basic block and used in other basic blocks. In that case the from_ctrl and to_ctrl would add a phi node to the SSA graph. (I suspect that may need more work anyway, since we make a lot of assumptions about the code being a DAG, etc.)

Copy link
Collaborator Author

@bmhowe23 bmhowe23 Sep 4, 2024

Choose a reason for hiding this comment

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

This made me wonder if we don't just want to insist that the linear-ctrl-form pass isn't run first?

It seems like decomposition could be run in pruned control form though.

This new promoteControls() is only run for specific decompositions that require it. Most decomposition passes do not require this. Requiring linear-ctrl-form be run before all decomposition passes, while functional, may be a bigger hammer than is currently required.

That being said - I'm ok with that ... I was just trying to make these decomposition passes support more flavors of the IR.

It seems like decomposition could be run in pruned control form though.

I agree. Note that the version in main already supports pruned control form for all except 4 decompositions, and these changes resolve those last 4, except for out-of-block use cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some expansion patterns may require the %ctrl qubit to "magically" transform to a target qubit. In those cases, we'd have to add from_ctrl and to_ctrl "inside" the pattern.

Yeah, this PR does that. Are you saying that you don't think this PR is doing that, or am I misinterpreting something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My comment above is descriptive of and in context of the general problem and not within the confines of what this PR does (or does not) do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This made me wonder if we don't just want to insist that the linear-ctrl-form pass isn't run first?

Just catching up on old reviews and I saw this one was open. Maybe I'm misinterpreting this, but this comment seems contrary to the spirit of #2145 (comment) because that comment seems to suggest that passes should handle all flavors of the IR while the one above (insist that the linear-ctrl-form pass is run first) makes it seem like it is ok to just assume that other passes will be run prior to this pass (and potentially after this pass to restore the control values). In fact, that other thread is the reason I made this PR, so I'm not sure what the next steps for this PR should be. Should we just change this pass back to not updating things when it sees quake.control types?

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 where this might be confusing. Let me see if I can't explain a bit more clearly.

  1. We don't want to reimplement the wheel in every pass. If there is a pass that does a particular transformation, use that pass instead of writing some new patterns that do almost the same thing. The reasons for this are: avoid duplication of effort, avoid introducing bugs, avoid long-term maintenance problems. For example, we really don't want a general folding pass, a pass that has custom folding of integer adds and multiplies as part of modifying certain calls, and a pass that custom folds calls to the math library as a side-effect of rescheduling blocks. The latter two passes have a lack of focus and instead of tackling the job they should, they are building a fiefdom of custom patterns that are unexpected and will likely, from experience, cause problems down the road.

  2. We want passes to tackle whatever input they are given without throwing errors in general. So, if we have a pass to rewrite high-level control flow to a primitive CFG, for example, the pass should run cleanly even if the IR is already represented in a primitive CFG. It just does nothing and the next pass can be applied. Or, if a new high-level construct was just added, the pass should just do what it can to convert the high-level structures it knows about and leave the new one there, which would indicate that someone needs to add the new op to the pass.

So to the point here, the pass could be written to be ignorant of the !quake.control type entirely. If it sees that type, it would just leave those ops alone and transform the ones it does know something about. Since there already is a general pass that eliminates !quake.control typed values, that pass might be used (since that is the focus and point of its existence) to perform that part of the work, while this pass has a narrower focus of doing decompositions in a reference/value semantics agnostic way. (i.e., only knowing about !quake.ref, !quake.wire.)

Hope that helps.

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.

2 participants