transformations: add qref/qssa conversions #2770

alexarice · 2024-06-24T16:42:32Z

Built on top of #2768

Adds two passes which convert back and forth between the qssa and qref dialects. Also adds filecheck tests that test each pass and roundtrip of the passes

I think the convert-qref-to-qssa is particularly hacky, with the saving the old operands of the operation into a tuple, running replace on the operands and then inserting back the old ones. Would be keen to hear better ways to achieve the same behaviour

codecov · 2024-06-24T16:48:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.80%. Comparing base (ea6f4d9) to head (45ea1f4).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2770      +/-   ##
==========================================
+ Coverage   89.79%   89.80%   +0.01%     
==========================================
  Files         376      378       +2     
  Lines       48079    48181     +102     
  Branches     7370     7372       +2     
==========================================
+ Hits        43172    43269      +97     
- Misses       3757     3762       +5     
  Partials     1150     1150

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AntonLydike

Would love to see docstrings on the patterns and/or passes that explain what they do!

AntonLydike · 2024-06-24T16:48:12Z

xdsl/dialects/qref.py

+    @property
+    @abstractmethod
+    def is_gate(self) -> bool:
+        """
+        Is this operation a gate
+        """


Can you add a docstring to explain why you need this helper?

I agree, also a more explicit description of what a gate is in this context.

AntonLydike · 2024-06-24T16:49:57Z

xdsl/transforms/convert_qref_to_qssa.py

+        # Create replacement operation
+        new_op: QssaBase = op.ssa_op()
+        # For gates there are no results to replace
+        new_results = () if new_op.is_gate else new_op.results


Couldn't this check be replaced with a check of "has the new_op results that are a qubit"?

is_gate should be equivalent to "The qubit-typed operands match one-to-one with the qubit-typed results".

This property could be removed and replaced by an actual check of this. Essentially the gates are the things we actually need to modify in the transform as they switch between having results and having no results depending on the dialect. Alloc and measure on the other hand are basically unchanged between dialects (apart from their types)

xdsl/transforms/convert_qref_to_qssa.py

xdsl/transforms/convert_qssa_to_qref.py

n-io

Looks good, I'd suggest to separate out dialect and transform code

n-io · 2024-06-25T07:25:51Z

xdsl/dialects/qref.py

+    @property
+    def is_gate(self) -> bool:
+        return False
+
    res: VarOpResult = var_result_def(qubit)


Style comment: Can you move res before the methods please

@AntonLydike I maybe incorrectly believed that this would cause some style check to fail, since it is done this way across all xdsl ops I've seen.

n-io · 2024-06-25T07:29:42Z

xdsl/dialects/qref.py

@@ -70,6 +94,17 @@ def print(self, printer: Printer):
 class HGateOp(QRefBase):
    name = "qref.h"

+    def ssa_op(self) -> qssa.HGateOp:


Best to keep dialect conversion functions in the converter rather than on the dialect.

I am unsure if this is desirable, as I don't see a way around having a big if isinstance chain in the converter without doing it this way. Another advantage of this method is that if a new operation is added, then the implementation of these two functions is forced

You could match on the specific op, create a new one, and move any shared code to a helper function., e.g. something along the lines of:

class ConvertHGateToQssaPattern(RewritePattern): @op_type_rewrite_pattern def match_and_rewrite(self, op: qref.HGateOp, rewriter: PatternRewriter): replace_with_ssa( op, qssa.HGateOp.create( operands=op.operands, result_types=(qssa.qubit,), attributes=op.attributes, ), rewriter, )

This still runs into the problem of not forcing the conversion to be written for new ops, though I guess a test could be written that ensures this

I kind of like the current setup. The only thing I might suggest is to have a QuantumGate trait instead of is_gate() but even that seems like a nit, and like less good a way to guarantee that each op will have to implement these methods when it's added. It also feels like the two dialects are coupled in a pretty fundamental way, which seems like an ok design decision, making the abstract method style of conversion quite natural. I'm not sure in what situation in the future we would regret merging this PR as-is.

This still runs into the problem of not forcing the conversion to be written for new ops, though I guess a test could be written that ensures this

This could also be a feature. Forcing a conversion to be made also implies an unwritten guarantee that there will always be a valid 1:1 conversion, and that neither of these dialects will ever hold concepts that are not directly convertible, now or in the future. I just wanted to draw attention to the fact that this tight coupling might be a rather committal decision. I don't fully know the plans for these dialects, so if you're happy then please go ahead and merge.

n-io · 2024-06-25T07:29:57Z

xdsl/dialects/qref.py

@@ -85,6 +120,17 @@ def __init__(self, input: SSAValue):
 class CNotGateOp(QRefBase):
    name = "qref.cnot"

+    def ssa_op(self) -> qssa.CNotGateOp:


n-io · 2024-06-25T07:30:05Z

xdsl/dialects/qref.py

@@ -102,6 +148,17 @@ def __init__(self, in1: SSAValue, in2: SSAValue):
 class CZGateOp(QRefBase):
    name = "qref.cz"

+    def ssa_op(self) -> qssa.CZGateOp:


n-io · 2024-06-25T07:30:12Z

xdsl/dialects/qref.py

@@ -119,6 +176,17 @@ def __init__(self, in1: SSAValue, in2: SSAValue):
 class MeasureOp(QRefBase):
    name = "qref.measure"

+    def ssa_op(self) -> qssa.MeasureOp:


n-io · 2024-06-25T07:30:38Z

xdsl/dialects/qssa.py

    name = "qssa.alloc"

+    def ref_op(self) -> qref.QRefAllocOp:


n-io · 2024-06-25T07:30:47Z

xdsl/dialects/qssa.py

    name = "qssa.h"

+    def ref_op(self) -> qref.HGateOp:


n-io · 2024-06-25T07:31:02Z

xdsl/dialects/qssa.py

    name = "qssa.cnot"

+    def ref_op(self) -> qref.CNotGateOp:


n-io · 2024-06-25T07:31:07Z

xdsl/dialects/qssa.py

    name = "qssa.cz"

+    def ref_op(self) -> qref.CZGateOp:


n-io · 2024-06-25T07:31:17Z

xdsl/dialects/qssa.py

    name = "qssa.measure"

+    def ref_op(self) -> qref.MeasureOp:


xdsl/dialects/qssa.py

Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>

alexarice · 2024-06-25T09:44:29Z

xdsl/transforms/convert_qref_to_qssa.py

+        # For gates we replace any other occurences of the original operands
+        # with the results of the new operation
+
+        old_operands = tuple(new_op.operands)


@math-fehr I requested a review to see if you knew a better way to do this section, but feel free to ignore if you're too busy

AntonLydike

Good stuff!

AntonLydike · 2024-06-26T09:18:42Z

xdsl/dialects/qref.py

+        """
+        Is this operation a gate?
+        Qref gates represent standard quantum logic gates.
+        They should have no results.
+        The results of the generated gate must be rewired when converting to the qssa dialect.
+        """


👍 Nice and concise docstring!

AntonLydike · 2024-06-26T09:19:57Z

xdsl/dialects/qssa.py

-class QubitBase(IRDLOperation, ABC):
-    pass
+class QssaBase(IRDLOperation, ABC):
+    """


I like this renaming, big fan 💪

AntonLydike · 2024-06-26T09:22:12Z

xdsl/transforms/convert_qref_to_qssa.py

+
+    def apply(self, ctx: MLContext, op: builtin.ModuleOp) -> None:
+        PatternRewriteWalker(
+            ConvertQRefToQssaPattern(), apply_recursively=False


Supernit: I'm 99% sure you don't need to specify apply_recursively=False here as it should not impact the result at all.

Every non-standard option specified unnecessarily introduces reviewer overhead.

I disagree, it has a performance impact, and should be added whenever the rewrites don't insert operations that could be recursively matched.

You are probably right, think I was worried about things being converted in the "wrong" order which could maybe cause verification problems (I'm not actually sure when the verifier is run)

The tests still succeed without it, so I guess it's just a performance thing

n-io

If we're happy with a tight coupling of these dialects (see discussion above) I'm happy to approve.

alexarice added the transformations Changes or adds a transformatio label Jun 24, 2024

alexarice requested review from superlopuh, AntonLydike and math-fehr June 24, 2024 16:42

alexarice self-assigned this Jun 24, 2024

AntonLydike reviewed Jun 24, 2024

View reviewed changes

n-io reviewed Jun 25, 2024

View reviewed changes

Base automatically changed from alexarice/qref-dialect to main June 25, 2024 09:25

superlopuh reviewed Jun 25, 2024

View reviewed changes

xdsl/dialects/qssa.py Outdated Show resolved Hide resolved

alexarice and others added 6 commits June 25, 2024 10:28

Add transforms

707fda8

Rename patterns

76f3cd1

Add docstrings

cfaa120

Reorder functions

96c3ce2

Update xdsl/dialects/qssa.py

7d23311

Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>

Add punctuation to docstrings

45ea1f4

alexarice force-pushed the alexarice/qref-transforms branch from 08fbe7e to 45ea1f4 Compare June 25, 2024 09:29

alexarice commented Jun 25, 2024

View reviewed changes

superlopuh approved these changes Jun 25, 2024

View reviewed changes

alexarice requested review from AntonLydike and n-io June 25, 2024 10:21

AntonLydike approved these changes Jun 26, 2024

View reviewed changes

n-io approved these changes Jun 26, 2024

View reviewed changes

alexarice merged commit 9d9bbdd into main Jun 26, 2024
10 checks passed

alexarice deleted the alexarice/qref-transforms branch June 26, 2024 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transformations: add qref/qssa conversions #2770

transformations: add qref/qssa conversions #2770

alexarice commented Jun 24, 2024

codecov bot commented Jun 24, 2024 •

edited

Loading

AntonLydike left a comment

AntonLydike Jun 24, 2024

superlopuh Jun 24, 2024

AntonLydike Jun 24, 2024

alexarice Jun 24, 2024

alexarice Jun 24, 2024

n-io left a comment

n-io Jun 25, 2024

n-io Jun 25, 2024

n-io Jun 25, 2024

alexarice Jun 25, 2024

n-io Jun 25, 2024 •

edited

Loading

alexarice Jun 25, 2024

superlopuh Jun 25, 2024

n-io Jun 26, 2024

n-io Jun 25, 2024

n-io Jun 25, 2024

n-io Jun 25, 2024

n-io Jun 25, 2024

n-io Jun 25, 2024

n-io Jun 25, 2024

n-io Jun 25, 2024

n-io Jun 25, 2024

alexarice Jun 25, 2024

AntonLydike left a comment

AntonLydike Jun 26, 2024

AntonLydike Jun 26, 2024

AntonLydike Jun 26, 2024

superlopuh Jun 26, 2024

alexarice Jun 26, 2024

alexarice Jun 26, 2024

n-io left a comment •

edited

Loading

transformations: add qref/qssa conversions #2770

transformations: add qref/qssa conversions #2770

Conversation

alexarice commented Jun 24, 2024

codecov bot commented Jun 24, 2024 • edited Loading

Codecov Report

AntonLydike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-io left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-io Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntonLydike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-io left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jun 24, 2024 •

edited

Loading

n-io Jun 25, 2024 •

edited

Loading

n-io left a comment •

edited

Loading