Skip to content

Single-source of truth GraphOpID generation - #3184

Draft
dime10 wants to merge 2 commits into
mainfrom
canonical_attr_printing
Draft

Single-source of truth GraphOpID generation#3184
dime10 wants to merge 2 commits into
mainfrom
canonical_attr_printing

Conversation

@dime10

@dime10 dime10 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Context:

Graph operation identifiers were independently assembled as delimiter-based strings in the Python frontend and C++ compiler. This made the encoding sensitive to formatting differences, some of which are identified below:
image

Description of the Change:

This PR replaces the existing GraphOpID encodings across the frontend and compiler with the canonical MLIR representation of a typed dictionary attribute. It does this by converting Python values to MLIR attributes first, assembling the full GraphOpID attribute structure (the "struct"), and then converting to string via the builtin MLIR attribute printers. The conversion to string could be eliminated entirely, but to limit the scope of the PR I'm keeping the string representation in all the users of the GraphOpID.

The struct schema is roughly based on the following:

  • stores the operation name under op,
  • uses positional arrays for wires and parameter types instead of dictionaries to keep the ID shorter,
  • adjoint and control modifiers are grouped under traits (this is mainly done to highlight the op name more, otherwise the adjoint field would be printed first by MLIR),
  • omits fields containing default or empty values to keep the ID shorter,
  • includes static data and UID as before.

All frontend producers use a shared build_graph_op_key helper. The DecomposableGate default implementation constructs the equivalent attribute in C++, but will still produce a string as a result.

Benefits:

  • Single source of truth for generating GraphOpIDs across Python and C++.
  • Less custom logic for the ID generation, which mostly reuses MLIR upstream printers.
  • Makes parameter and wire names irrelevant to operator identity.
  • Modifier handling now updates structured fields instead of adding or removing textual prefixes.

Possible Drawbacks:

To limit the scope of the PR, I didn't change any consumer code of the GraphOpID, so it still expects strings. As a result, we still have a conversion of the structured dictionary representation to string, and on the other side a parsing step currently used for two reasons: generating modified IDs (e.g. adding an adjoint), and obtaining a simplified name for gate set matching. Ideally, the ID should be fully opaque, and once generated not touched except to test equality. Hopefully something we can further improve in the future.

@dime10
dime10 requested review from kipawaa and paul0403 September 3, 2026 20:22
Comment thread mlir/include/Quantum/IR/QuantumInterfaces.td
@kipawaa

kipawaa commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

assembling the full GraphOpID attribute structure

One of my inspirations for generating a single string ID rather than a structure was because I intended for it to be static and opaque, i.e. it was intended to only be used for equality comparison. Do we need greater functionality than this? Not that a structure is worse, in fact it might be better in terms of performance, but I worry that this will encourage more manipulation of IDs (similar to what we see with adjoint/control modifiers in the frontend right now). My goal would be to generate new IDs from operator data rather than manipulate ID data, though maybe at this point I'm holding on to an idea that will only get in our way...

@dime10

dime10 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

assembling the full GraphOpID attribute structure

One of my inspirations for generating a single string ID rather than a structure was because I intended for it to be static and opaque, i.e. it was intended to only be used for equality comparison. Do we need greater functionality than this? Not that a structure is worse, in fact it might be better in terms of performance, but I worry that this will encourage more manipulation of IDs (similar to what we see with adjoint/control modifiers in the frontend right now). My goal would be to generate new IDs from operator data rather than manipulate ID data, though maybe at this point I'm holding on to an idea that will only get in our way...

I agree that the ID should be opaque (at least for the code, inspectability aside), however that is already not the case in two instances: generating adjoint/control versions of decomposition reaches in and manipulates strings, and generating the shortened name to compare against the gate set. I would support moving towards never touching the ID once it's generated.

In this PR, I don't really change that fact. The ID is generated by going through a structured attribute first, but then it is serialized to string. There is still the parsing of the string for the two use-cases I mention above. Perhaps we can fix those first.
I'm also not convinced this particular implementation in this PR is the way to go, but I do think the core change of using the attribute printer in both frontend and compiler to generate identical IDs across the board and minimize custom serialization code is the way to go.

@kipawaa

kipawaa commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

@dime10 Sounds like we're on the same page, and I agree that sending everything to MLIR attributes then serializing for consistency is ideal. Let's go ahead with this (I'll review shortly) and I can try to address the string manipulation in a separate PR :)

@dime10

dime10 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

@dime10 Sounds like we're on the same page, and I agree that sending everything to MLIR attributes then serializing for consistency is ideal. Let's go ahead with this (I'll review shortly) and I can try to address the string manipulation in a separate PR :)

I don't really like how many changes there are in this PR (although a lot of them are tests), so I am experimenting with an alternative implementation. Not sure I've landed on something I really like yet 🤔

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.

3 participants