HLS fix to not synthesize instructions already supported #13417
Conversation
|
One or more of the following people are relevant to this code:
|
alexanderivrii
added
the
Changelog: Bugfix
The control-flow operations such as 'for-loop` are considered to be a part of 'basis_gates`, yet they need to be recursively synthesized.
Pull Request Test Coverage Report for Build 11813177643Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
mtreinish
added
the
stable backport potential
There was a problem hiding this comment.
Just to double check: this bug with PauliEvolution only came up in 1.3, but already in 1.2 this was wrong. However, we didn't notice with PauliEvolution gate though, since we didn't have plugins, but it could've been triggered with something else, right?
There was a problem hiding this comment.
Yes, exactly!
Co-authored-by: Julien Gacon <gaconju@gmail.com>
There was a problem hiding this comment.
LGTM for this fix, thank you!
However, I’m mildly unhappy about that special treatment of control flow… there’s somehow a different meaning to basis gates. Usually a name being in basis_gates means we can run it, but for control_flow that’s not quite true, since we still need to synthesize it’s insides 🤔
Thanks @Cryoris! I completely agree, I would also like to see a better separation between gates and between control-flow logic. However, for the purpose of the bug fix. Is there anything specific I should do? |
|
No I agree this is the correct fix for now, this was more a comment to discuss this in general later on 🙂 |
* HLS fix to not synthesize instructions already supported * reno * fix for control-flow-operations The control-flow operations such as 'for-loop` are considered to be a part of 'basis_gates`, yet they need to be recursively synthesized. * using faster is_control_flow check * Update test/python/transpiler/test_high_level_synthesis.py Co-authored-by: Julien Gacon <gaconju@gmail.com> --------- Co-authored-by: Julien Gacon <gaconju@gmail.com> (cherry picked from commit b9d5c9c)
…3441) * HLS fix to not synthesize instructions already supported * reno * fix for control-flow-operations The control-flow operations such as 'for-loop` are considered to be a part of 'basis_gates`, yet they need to be recursively synthesized. * using faster is_control_flow check * Update test/python/transpiler/test_high_level_synthesis.py Co-authored-by: Julien Gacon <gaconju@gmail.com> --------- Co-authored-by: Julien Gacon <gaconju@gmail.com> (cherry picked from commit b9d5c9c) Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
…kit#13417)" This reverts commit b9d5c9c.
* v0 of comm checker * dangling print * games with matrices * Revert "HLS fix to not synthesize instructions already supported (#13417)" This reverts commit b9d5c9c. * use custom mm1 and mm2 * expose tol * rename tol to approximation_degree * note: this is not global phase correct! * don't be phase agnostic * additional tests these cover cases where we accumulate roundoff errors * Increase threshold, better consistency - increased the threshold for value comparisons to 16 EPS - use same functions in RemoveIdentityEquiv and CC checker - add reno - add detailed docs - add some tests for above thresholds * Python-space approx degree oversight * update threshold to 1e-12 which is consistent with the 2qWeylDecomp and still stricter than Qiskit 1.x * review comments * inline and rm comments
Summary
Fixes #13412.
Needs to be backported to 1.3.0.
Details and comments
Previously
HighLevelSynthesissynthesized an instruction for which a synthesis plugin was available, even when the instruction was already considered supported (either supported by the target or a part ofbasis_gates). This commit fixes this behavior.Update: the control-flow operations (e.g.
for-loop) belong tobasis_gatesbut need to be recursively synthesized.Even though the problem was only discovered for "PauliEvolution" gates also added for 1.3, the incorrect behavior for other high-level gates existed before, so I added release notes. I was not sure if they should be under
releasenotes/notes/1.3or not.