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

[WIP] Remove condition/c_if, duration, and unit from instructions #13506

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mtreinish
Copy link
Member

Summary

This commit removes the per instruction attributes for condition, duration, and unit. These attributes were deprecated in 1.3.0. Besides the label these attributes were the only mutable state for singleton instructions and removing them simplifies what needs to be tracked for instructions in both Python and rust. The associated methods and classes that were previously dependent on these attributes are also removed as they no longer serve a purpose. The removal of condition in particular removes a lot of logic from Qiskit especially from the transpiler because it was a something that needed to be checked outside of the normal data model for every operation.

This PR simplifies the representation of this extra mutable state in Rust by removing the ExtraInstructionAttributes struct and replacing it with a Option<Box<String>>. The Option<Box<String>> is used instead of the simpler Option<String> because this this reduces the size of the label field from 24 bytes for Option<String> to 8 bytes for Option<Box<String>> with an extra layer of pointer indirection and a second heap allocation. This will have runtime overhead when labels are set, but because the vast majority of operations don't set labels the tradeoff for optimizing for the None case makes more sense. Another option would have been to use Box<str> here which is the equivalent of Box<[u8]> (where String is the equivalent to Vec<u8>) and from a runtime memory tradeoff would be a better choice for this application if labels were commonly used as labels aren't growable. But that requires 16 bytes instead of 8 and we'd be wasting an additional 8 bytes for each instruction in the circuit which isn't worth the cost.

Details and comments

TODO:

  • Fix scheduling pass updates
  • Remove remaining c_if usage in tests (there's >200 instances of it still)
  • Fix any failing tests not caused by the above 2 (hard to see with ~300-400 failures)
  • Finish writing release notes
  • Do some tuning/profiling to see if we can optimize things a bit more after the removal

@mtreinish mtreinish added on hold Can not fix yet Changelog: API Change Include in the "Changed" section of the changelog Changelog: Removal Include in the Removed section of the changelog Rust This PR or issue is related to Rust code in the repository mod: transpiler Issues and PRs related to Transpiler mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Dec 1, 2024
@mtreinish mtreinish added this to the 2.0.0 milestone Dec 1, 2024
This commit removes the per instruction attributes for condition,
duration, and unit. These attributes were deprecated in 1.3.0. Besides
the label these attributes were the only mutable state for singleton
instructions and removing them simplifies what needs to be tracked for
instructions in both Python and rust. The associated methods and classes
that were previously dependent on these attributes are also removed as
they no longer serve a purpose. The removal of condition in particular
removes a lot of logic from Qiskit especially from the transpiler
because it was a something that needed to be checked outside of the
normal data model for every operation.

This PR simplifies the representation of this extra mutable state in
Rust by removing the `ExtraInstructionAttributes` struct and replacing
it with a `Option<Box<String>>`. The `Option<Box<String>>` is used
instead of the simpler `Option<String>` because this this reduces the
size of the label field from 24 bytes for `Option<String>` to 8 bytes
for `Option<Box<String>>` with an extra layer of pointer indirection
and a second heap allocation. This will have runtime overhead when
labels are set, but because the vast majority of operations don't set
labels the tradeoff for optimizing for the None case makes more sense.
Another option would have been to use `Box<str>` here which is the
equivalent of `Box<[u8]>` (where `String` is the equivalent to
`Vec<u8>`) and from a runtime memory tradeoff would be a better
choice for this application if labels were commonly used as labels
aren't growable. But that requires 16 bytes instead of 8 and we'd be
wasting an additional 8 bytes for each instruction in the circuit which
isn't worth the cost.
@nonhermitian
Copy link
Contributor

Is there a replacement method for getting circuit timing if duration is removed? It is somewhat critical to have a way to determine circuit timing for a variety of reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog Changelog: Removal Include in the Removed section of the changelog mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library mod: transpiler Issues and PRs related to Transpiler on hold Can not fix yet Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants