Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3393 +/- ##
=======================================
Coverage 96.21% 96.22%
=======================================
Files 176 176
Lines 21583 21580 -3
Branches 2160 2158 -2
=======================================
- Hits 20766 20765 -1
+ Misses 631 630 -1
+ Partials 186 185 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context:
Rework various operand sorting regarding decomposition to have a single source of truth.
Currently, an operator's arguments are written down in three different orders, and different parts of the code read different ones:
Operator2._primitive_bind_call. The compiler also hands operands to a rule in this order, verbatim, during--decompose-lowering.param_mapandqubit_mapare MLIR dictionary attributes, so MLIR always sorts them by name. For this reason, the frontend'sGraphOpIDparsers sorted their entries by name.llvm::StringMaporder. The C++getWireLens()andgetDynamicShape()returnllvm::StringMap, which iterates in non-deterministic order. That order is what the on-demand path passes to Python, since it rebuilds python callback args via theDecomposableGate::get*()methods.A rule only works if its input layout matches the operands it receives. Each order was used somewhere, so any operator whose declaration order differs from alphabetical order could break. This produced four bugs:
ordered_kwarg_namessorted the paramsGraphOpIDhashed them in sorted orderStringMapiteration orderDescription of the Change:
Python changes:
It is important to note that we have been operating under a misconception this whole time: just because MLIR dictionary attribute is sorted, does not mean we need to send a sorted dictionary into the dictionary attribute builder.
In short, this PR aims to remove all manual alphabetical sorting of the operands except when building the ID string in the frontend, since that's the only place that actually needs sorting (to match ID strings with the ID string produced from the compiler).
cpp changes:
The
DecomposableGate::get*()methods now returnllvm::Smallvector<std::pair<std::string, ...>>instead ofllvm::StringMapto guarantee iteration order when rebuilding on-demand rule python callback args.Noteworthily,
OperatorOpmust retrieve the operand order from the dinctionary attribute order. So a new helper,getEntriesInOperandOrder, readingparam_map/qubit_mapand stable-sorts the entries by their first operand index, was added to achieve this.Benefits:
Correctness.
SemiAdderworks correctly.Single source of truth for operand order regarding decompositions.
Possible Drawbacks:
None
Related GitHub Issues:
Fixes #3337
[sc-133791]
Assisted by Opus 5.5