FIR Transforms#3187
Conversation
| /// Pin-based fallback for callable args containing closures with captures. | ||
| /// | ||
| /// Seeds concrete (non-arrow-input) callables into the entry for reachability, | ||
| /// pins arrow-input callables and the target for DCE survival, and lets | ||
| /// `fir_to_qir_from_callable` handle specialization at QIR generation time. | ||
| fn prepare_codegen_fir_from_callable_args_pinned( | ||
| package_store: &PackageStore, | ||
| callable: qsc_hir::hir::ItemId, | ||
| _args: &Value, | ||
| capabilities: TargetCapabilityFlags, | ||
| mut concrete_callables: FxHashSet<qsc_fir::fir::StoreItemId>, | ||
| ) -> Result<CodegenFir, Vec<Error>> { |
There was a problem hiding this comment.
Not using _args here? Maybe can delete the _args parameter?
| assigner.set_next_stmt(StmtId::from(max + 1)); | ||
| } | ||
|
|
||
| // NodeId — scan callable and spec decls |
There was a problem hiding this comment.
As it turns out, NodeId is only used in three places in FIR where it is set as an id, but then never read. I think we can drop it from FIR entirely.
| @@ -65,6 +87,38 @@ fn test_single_qubit() { | |||
| ); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_explicitly_annotated_single_qubit_rewrite_preserves_binding_name_and_types() { | |||
There was a problem hiding this comment.
This test and the one above are effectively identical... they don't verify anything different just use different mechanisms to do so.
| let qir = generate_qir_from_ast( | ||
| package, | ||
| unit.source_map, | ||
| unit.profile.unwrap_or(Profile::Unrestricted), |
There was a problem hiding this comment.
I get that this is only used for tests, but it seems odd for the default for QIR generation to be a profile that we know will fail QIR generation. Should this be Adaptive_RIF?
| Value::Array(vs) => { | ||
| let mut lowered_ids = Vec::with_capacity(vs.len()); | ||
| for v in vs.iter() { | ||
| lowered_ids.push(lower_value_to_expr(package, assigner, v, callable_types)); | ||
| } | ||
| let elem_ty = lowered_ids.first().map_or(qsc_fir::ty::Ty::Err, |id| { | ||
| package.exprs.get(*id).expect("just inserted").ty.clone() | ||
| }); | ||
| ( | ||
| qsc_fir::fir::ExprKind::Array(lowered_ids), | ||
| qsc_fir::ty::Ty::Array(Box::new(elem_ty)), | ||
| ) | ||
| } | ||
| Value::Range(r) => { |
There was a problem hiding this comment.
Since we know some folks invoke Q# callables with very large arrays (RE and chemistry scenarios, for example), we may pay a high cost of generating a large array literal into the synthetic entry expression only for it to be mostly ignored (since the synthetic entry is used for analysis in the passes and not execution). It might be worth trying to detect this case and avoid emitting constant arrays when not needed.
| /// 3. Asserts the two results match (both succeed with equal values, or | ||
| /// both fail). | ||
| #[cfg(test)] | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Looks like this allow isn't needed anymore.
| testutil = ["qsc_frontend", "qsc_hir", "qsc_passes"] | ||
|
|
||
| [dev-dependencies] | ||
| qsc_fir_transforms = { path = ".", features = ["testutil"] } |
There was a problem hiding this comment.
We started talking about this, and I see why it's needed now to make the testutil functionality available via the public API to scenario tests. It seems like there might be another way around that (maybe moving the tests, maybe moving the utils), but it's not critical for this PR.
…e values as input. Co-authored-by: Copilot <copilot@github.com>
…r codegen. Use pinning as a fallback for stateful captures Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
…e construction. Package store cloning is done via derive macro
3203541 to
dcecb3d
Compare
There was a problem hiding this comment.
Okay, am I hallucinating or did we already have a Q# pretty printer that we used for OpenQASM tests?
| /// `CallableImpl` input counts) would flip that implicit assertion into a skew | ||
| /// panic before the explicit `ComputeKind` check below is reached. | ||
| #[test] | ||
| fn flag_fallback_value_kind_after_dynamic_scope_return() { |
There was a problem hiding this comment.
this test could make use of the pattern used in callables::check_rca_for_callable_block_with_dynamic_unreachable_binding, specifically having check_callable_compute_properties validate the callable properties via expect.
| @@ -1363,3 +1363,24 @@ fn call_to_intrinsic_operation_that_takes_qubit_array_should_fail() { | |||
| } | |||
| "}); | |||
| } | |||
|
|
|||
| #[test] | |||
| #[should_panic( | |||
There was a problem hiding this comment.
I'm just noticing that this pattern (which other tests in this file use) is unnecessary: there is a helper that checks for partial evaluation errors: get_partial_evaluation_error. This test and the three above it could be converted to that pattern rather than should_panic which makes test output cleaner and continues the expect-test patterns.
|
|
||
| impl InvariantLevel { | ||
| /// Returns `true` when this level is at or after monomorphization. | ||
| fn is_post_mono_or_later(self) -> bool { |
There was a problem hiding this comment.
It occurs to me that since the InvariantLevel enum is strictly ordered, you could derive PartialOrd and Ord and then accomplish these checks via simple >= checks.
Summary
This PR adds FIR passes to enable broader code generation scenarios.
QIR does not support:
RIR doesn't currently support:
return_unifytries to remove this constraint but there are some odd things we can't deal with.The passes peel each unsupported piece off in the pipepline.
Aside from the passes, this PR also tries to unify how the code goes through RCA and codegen compilation. There are some side effects which leak into circuits as we have to generate new functions as part of the passes that we don't necessarily want reflected in the circuit representation.
Suggested Review Assignment
Crate organization
Integrating
qsc_fir_transformswithqsc_passeswas going to make the PR look much bigger with a lot of moved files. My plan was to mergeqsc_fir_tranformsintoqsc_passesand organize them byHIRandFIR. This way we'd have a clean refactoring PR with no functional changes. This PR is already very large and I thought this integration was just too much to add.Error types
ErrorKind::FirTransformwill merge withErrorKind::Passinsource/compiler/qsc/src/compile.rsin a follow up PR unless we want to differentiate betweenHIRandFIRpasses at this level. We may want to differentiate at theqsc_passeslevel but merge them at this level as diagnostic transparent pass errors. The same follows forError::PassandError::FirTransforminsource/compiler/qsc/src/interpret.rs.Interpret
This crate has two major changes. First the codegen module has a lot of added code for preparing the compilation. When we have both callables with interpret values (which may themselves be callables/structs/tuples which may contain the same complicated values) and entry expressions, we need to update the compilation in very different ways. For callables we need to effectively generate a new synthetic entry expr which can use the interpreter values. There is a case when dealing with closures where we need to partially abandon this pass and use a fallback of pinned non-entry-reachable items which are passed into the pipeline for processing. Entry expresssions are the easy path and just work as normal heading into the pipeline.
The interpret module does some setup work to help the codegen module.
The openqasm module has some fixes that are related to profile not being plumbed correctly. We weren't handling the user's specified profile and the codes annotated profile correct when used together and making the assumtion that if it was missing from the code that the profile was unrestricted. You'll see this update propagated into the Python and parser.
qsc_fir
The big addition here is the assigner. The FIR transforms do a lot of code generation and mutation, but it is additive. When we are generating new code, we need consistent, non-overlapping ids, for blocks, exprs, items, etc. This assigner update allows us to create an assigner from a package which finds the next values of each id needed so that we can safely allocate.
Testing
Some tests have been added to seemingly random places. These tests were added after I broke things and didn't know as no tests were failing. They are there to prevent regressions.
New instruction
fremThe
freminstruction is added to support OpenQASM dynamic angle support. Hopefully it will be added to the adaptive profile soon. Without this instruction we cannot do runtime angle calculations in OpenQASM as theangletype requires this computation.Codegen
The qir codegen now requires RCA to have been already done before calling into fir_to_rir. We had too many places where we were or were not running RCA and then having to run it after the fact. This made it difficult to know when RCA was actually taking place. There are a few refactorings around this so that we have this more consolidated, but we might want to take a deeper step towards unifying in the future.
Circuits
Transformed callables are cloned into the user package. In order to maintain the same visualization as before, we have to detect whether we are in a 'synthetic' callable context so that we don't emit the call as a grouping context.
Partial eval
There is a lot of code in partial eval for dealing with return statements. I've documented
source/compiler/qsc_partial_eval/src/evaluation_context.rsindicating that this is no longer required, but such a refactoring adds a lot of risk and code change which is better defferred to a follow up PR.LLVM IR Changes
There are a few test files which are updated as the passes enable better code generation options that were impossible to handle before and were forced to be inlined.
Performance
The FIR transforms can be made faster, but they take less than 1/5 the time of the regular compilation and 1/15 as much time as RCA, so they are fast enough for the moment.
Random looking changes
source/compiler/qsc_frontend/src/closure.rs - documented here as the exact shape of closures has downstream effects and we can't vary from this structure without also changing many other sites.
source/compiler/qsc_frontend/src/resolve.rs - fixes a bug in type resolution where supplying an explicit
: Qubittype onusestatements leads to the var's pat type being error.