[Stretch] Support const classical expressions.

[kevinhartman]

Summary

Adds support for const classical expressions. This is necessary to properly represent Duration and Stretch data types, which are always constant expressions.

Everything pretty much works the same, except expr.Expr has gained a property called const which indicates its const-ness. An expression is constant if all of its operands are constant. The Value expression is always constant, while Var is (for now) non-const.

Details and comments

Originally, this PR had introduced const-ness as a property of the type system, itself. This was done to mirror what we do in the QASM3 importer, and was what we (@jakelishman and I) thought was the best approach at the time. However, after the initial review, it became clear that const-ness as a property of the expression (Expr) is a much more natural fit for Qiskit's classical expression system.

With const-ness as a property of Expr, we can get away with tracking const-ness entirely in the initializers of our expression classes, which simply set their own const-ness based on the const-ness of their operands. The Value and Var nodes act as our base-case since their const-ness is known, statically. This also means that no special handling is needed for QPY.

The original design notes of the type-based const-ness approach is below for posterity, but it is no longer true:

• expr.lift now supports lifting non-classical resource scalars into values with const types, via parameter try_const=True.
• Type ordering now considers a const type to be less than its non-const version.
• An implicit Cast node is generated when a const expression must be coerced into a non-const context.
• An expression with a const type is never an l-value, even if it is a Var.
• EDIT: I decided not to make this decision right now, and to instead block const variables from being added to the circuit via add_var and add_input.
• Binary operations will attempt to lift inputs with matching const-ness.
  • A Cast node is inserted if both operands are Expr and only one of them is const.
• Index and shift operations will lift their RHS to match const-ness of the LHS.

To-do

• Needs QPY tests of const types used with old versions and with current version.

[kevinhartman]

Unlike binary bitwise, a Cast node is currently inserted if both operands are Expr and only one of them is const. This is an effect of the coercion to Bool. Perhaps this needs revisiting.

If the consensus is that it's fine for two binary operands to differ in const-ness, I can also introduce a CastKind.NON_CONST and allow the caller to apply a cast node based on the context. My impression would be that we wouldn't need one anywhere for our current use cases if binary operands don't need the same const-ness.

EDIT: In offline conversations, we decided that having a cast node is the most consistent choice for our expression system, so there is always an implicit cast node to take a const-typed expression down to its non-const counterpart.

[qiskit-bot]

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

• @Qiskit/terra-core
• @mtreinish
• @nkanazawa1989

[jakelishman]

Minor comments aside, this looks solid to me, thanks. I wouldn't say I feel 100% confident on whether const is part of the type or part of the expression (I know we talked about it offline), but I think the way you've done it is the better way to go for us - I think it's easier on us later to have it all very explicit in the structure.

[jakelishman]

Is there a concern that literals shouldn't always be const (also, I think we mean "literal", not "scalar")?

[kevinhartman]

There are two things I was thinking about here:

1. In OpenQASM, the operands of a binary expression are always first cast to the greater of the two types. Following that pattern, if literal types are always const, then that would mean we'd always need a cast node in a binary operation between a Var and a literal, which felt weird.
2. Lifting literals to non-const by default was the more API compatible choice (e.g. we can still write expressions that don't use const types with pre-14 QPY).

The way we've got it in this PR, expr.lift will lift literals to non-const by default. But, any of our API surface that we consider to be accepting an expression in a "const context" can lift its argument as const explicitly with try_const=True. All binary operations where one side is already a const-typed expression do this, and perhaps we'd do this in an eventual QuantumCircuit.add_const_var(...) as well.

[jakelishman]

tbh, I don't think OpenQASM's type system (especially around const) is that well defined. I guess my question is whether we think "literals may be either type const uint[n] or uint[n]" is more or less weird than "we have implicit Cast nodes that only throw away const-ness".

I'm fine to leave it like this - it's probably the best choice already, and if there's a problem, there's probably a lot bigger demons lurking in our type system elsewhere, since the initial thing was kind of just put together on vibes.

[kevinhartman]

literals may be either type const uint[n] or uint[n]

This might just be an artifact of having the const-ness as part of the type rather than the expression, since it allows a Value expression to have a non-const type. If const-ness were instead an attribute of expression, then Value could be made to always report itself as const.

Just a thought. It's probably too late to change this, but I'm happy to try if you think it's the right idea (and it's permitted by the feature freeze lol).

[jakelishman]

For posterity: having the join of the two types not be equal to one of the two isn't a problem for a type system (for example, C has this property, and anything with ad-hoc sum types like Python's Union can do in certain contexts too), I was more meaning I worried we had assumptions elsewhere that it held.

But since 8021e00, it's no longer relevant anyway, so no worries.

[jakelishman]

Possibly this is clearer as

non-const :class:`~.expr.Var` nodes are l-values ...

since the const bit is pretty fundamental here.

[kevinhartman]

I believe this comment is actually no longer valid, since I decided not to "decide" const-typed expressions can never be lvalues. Originally, I'd changed the implementation of is_lvalue to return False for any const-typed expression.

The reason for my deferral was that it seemed reasonable to me to use a store instruction to initialize a const variable, even though we block it at the moment. I'm not sure if it's valid in QASM to initialize a constant without an initializer, e.g.:

duration d;
d = 1200dt;

[jakelishman]

Yeah, I don't know if that's valid or not. But tbh, even if it is valid, I'd just rather not support it in Qiskit - a few languages let you forward-declare storage, and initialise it in some scope, as long as the set of initialising statements dominates the set of statements using the variable. Rust is one of those - it's valid to have let x; and then assign to x in some scope, provided the structure of the program guarantees that x is assigned if it's used.

I'd rather just not support that in Qiskit, just to reduce complexity. If we gain more complex handling for classical control flow analysis later, we can always revisit.

[kevinhartman]

Sounds good to me. I thought of the let x; pattern in Rust too, but then I realized that Rust doesn't allow const x;, which is closer to the semantics I'm alluding to here—it requires an inline initializer.

[jakelishman]

Maybe want to be slightly clearer in this that this modifies the meanings of the type keys compared to version 9. Line 392 needs double backticks around _Bool const.

[kevinhartman]

I've changed this around a bit in the later PRs. If it's alright, it might be easier for us to keep this as is and then make sure that the final PR reflects everything in a cohesive and complete manner.

[jakelishman]

I'm fine to defer, but can you open an issue tagged for the milestone about what you mean, just to remind us to do it as part of the documentation in the final PR?

[kevinhartman]

Added issue #13927

[jakelishman]

Issue #13927 is obsoleted by 8021e00, I think?

[jakelishman]

Well, if not obsoleted, then at least the changes to QPY should now be localised to a later PR I think.

[jakelishman]

Thanks for this - I think it's about ready to land, bar the merge conflict and the couple of minor comments. Two of them are just musings about the effects of the type system and non-actionable, one is super minor documentation, and the other I think is a missing bit of logic in the logical-operation type inference.

I feel like there's an easy chance I could have missed things in the tests, but that's what bugfixes are for haha, and it seems correct.

[kevinhartman]

Thanks for the review, @jakelishman.

As we've discussed offline, your comments got me thinking that tracking const-ness as a property of Expr rather than as part of our type system would be a more natural fit for Qiskit.

For others, my reasoning was:

The main idea is that we move const to Expr and let the constructors of Expr subclasses initialize it. Value initializes const to True, always. Var initializes const to False, always. Operation expressions initialize it based on their operands. E.g. for Binary, we set self.const = left.const and right.const.

The benefits are:

1. We don’t need to change the logic of lift at all, and there’s no fussing with const-ness needed (no need to decide a default for try_const). And, there’s no future need to contextually lift something with const-ness in mind. An expression is const if all of its parts are, simple as that.
2. We don’t have the issue with ordering where the join of two types can be some third type.
3. Literals are always const, since Value is always const.
4. No QPY changes are needed whatsoever, since the expression tree is put back together from the bottom up, starting with the leaf nodes Var and Value whose const-ness we know statically.
5. Zero changes are needed to the expression constructors.
6. We avoid a potential weirdness that you’ll see in the timing types PR, which is that Stretch and Duration are always const. If some kind of runtime duration gets added later, durations will be const by default, but opt-in non-const, which is backwards from other types.

The branch has now been updated with these changes. This seems like a big change, but really we've just moved an attribute from one class to another, and everything else is largely removals.

[jakelishman]

I think you're right, this will end up a lot cleaner, and an expression being const doesn't oblige us to evaluate it at compile time. I think this somewhat formalises Value to mean "immediate value", which I don't think I connected the dots on in the first iteration of the expr system, but I think it makes a lot of sense: Var is associated with memory locations; Value is immediate values; and then we've got a nice framework to fit stretches and durations into.

Super minor typographical comments only.

[jakelishman]

Suggested change

	us to enforce that expressions in certain contexts must be possible to evaluate at compile-time.
	us to enforce that expressions in certain contexts must be possible to evaluate at compile time.