Remove init peephole optimization discrete basis check

[mtreinish]

Summary

In #12727 a check was added to the default init stage's construction to avoid running 2q gate consolidation in the presence of targets with only discrete gates. However the way the target was being used in this check was incorrect. The name for an instruction in the target should be used as its identifier and then if we need the object representation that should query the target for that object based on the name. However the check was doing this backwards getting a list of operation objects and then using the name contained in the object. This will cause issues for instructions that use custom names such as when there are tuned variants or a custom gate instance with a unique name.

While there is some question over whether we need this check as we will run the consolidate 2q blocks pass as part of the optimization stage which will have the same effect, this opts to just fix the target usage for it to minimize the diff. Also while not the explicit goal of this check it is protecting against some bugs in the consolidate blocks pass that occur when custom gates are used. So for the short term this check is retained, but in the future when these bugs in consolidate blocks are fixed we can revisit whether we want to remove the conditional logic.

Details and comments

[qiskit-bot]

One or more of the following people are relevant to this code:

• @Qiskit/terra-core

[coveralls]

Pull Request Test Coverage Report for Build 10253951061

Details

• 9 of 10 (90.0%) changed or added relevant lines in 2 files are covered.
• 11 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.01%) to 89.738%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/preset_passmanagers/builtin_plugins.py	4	5	80.0%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	94.02%
crates/qasm2/src/lex.rs	4	91.73%
crates/qasm2/src/parse.rs	6	97.61%

Totals
Change from base Build 10222290210:	-0.01%
Covered Lines:	67319
Relevant Lines:	75017

---

💛 - Coveralls

[jakelishman]

I'm getting more confused by the logic of this check. The name of it (is_one_op_nondiscrete) implies any-like short-circuiting logic, except there are points that we short-circuit to return False and points that we short-circuit to return True?

I'm not sure what's up - maybe the function is just named incorrectly - but something still feels wrong about its logic here.

[jakelishman]

This documentation comment isn't accurate now (though I think it wasn't 100% perfect before either).

[mtreinish]

Fundamentally the check is not solely what its name indicated before or now; it really was "are_any_gates_not_discrete_and_are_all_operations_known" or something like that. Realistically, the piece that's actually needed here is actually the known check; it's protecting us against issues running ConsolidateBlocks with custom operations that it can't reason about. I need to open another bug about ConsolidateBlocks crashing with custom opaque gates so we can track fixing that too..

[jakelishman]

Given the problems we've had here, I'm leaning a bit more towards making the check only the bit that's actually required to match the previous behaviour, since the more superfluous conditions we're adding onto the top, the harder it is to read, and as seen here and before, the easier it is for us to have mistakes in it.

I am still roughly interested in us having a better story for discrete-basis translation in the future, but that's very much a future thing (probably we need a performant TemplateMatching before it becomes entirely feasible for optimisations).

[mtreinish]

Ok, I pivoted the PR to just remove the check and fix the underlying bug in ConsolidateBlocks it was protecting against.

[jakelishman]

Thanks - this does seem like the better way to go overall.