Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
| /// Note: The `Return` variant is vestigial for the production pipeline. | ||
| /// The `return_unify` FIR transform pass eliminates all `ExprKind::Return` | ||
| /// nodes before partial evaluation runs. However, partial eval unit tests | ||
| /// bypass FIR transforms and evaluate raw FIR, so the `Return` variant | ||
| /// and its handling code remain for test compatibility. |
There was a problem hiding this comment.
Definitely out of scope for this PR, but it would make me so happy to remove this, clean up partial eval, and update how the tests run. Handling of embedded nested returns was a necessary evil that the new passes can let us finally get away from. Maybe worth a tracking issue to come back to this?
There was a problem hiding this comment.
There is another option if we want to support multiple returns like in the adaptive ext, then we need to still handle them and turn of return_unify
| let qsc_hir::hir::ExprKind::Call(callee, args) = &entry.kind else { | ||
| return None; | ||
| }; | ||
| let qsc_hir::hir::ExprKind::Tuple(items) = &args.kind else { | ||
| return None; | ||
| }; | ||
| if !items.is_empty() { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
This initially confused me, as it seems to reject any entry expression that is not a call with no arguments, but looking at the context of where this function is called I can see that matches the use. Might be worth a clarifying comment on the function itself what kind of expressions it returns and what kind it skips.
| /// | ||
| /// Applies compiler passes (monomorphization, defunctionalization, UDT erasure, etc.) | ||
| /// to produce backend-ready FIR satisfying full invariants. | ||
| pub fn run_backend_pipeline( |
There was a problem hiding this comment.
nitpick: I'm still not crazy about the name "backend" here since that is an overloaded term these days for us. Maybe the right thing to do here is either rename the run fir passes functions that today just run RCA to be honest about the fact they are specific to that so that we can make these functions about fir passes, or embrace the idea that RCA is an fir analysis pass and just lump them all together. What do you think?
There was a problem hiding this comment.
Renamed a bunch of stuff in 8d0be01 for you.
| qsc_fir::ty::Ty::Infer(_) | ||
| | qsc_fir::ty::Ty::Param(_) | ||
| | qsc_fir::ty::Ty::Prim(_) | ||
| | qsc_fir::ty::Ty::Udt(_) |
There was a problem hiding this comment.
this needs an update to handle cases where arrow types are hidden inside of UDTs.
There was a problem hiding this comment.
First part is done for this a09718c
Second part is in progress. The pinned non-entry reachable callables need to be added to the pipeline in order for eventual code generation. Right now it is assumed that they will go away during partial evaluation.
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
…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>
2 participants
This is a draft and this description will be filled in shortly. Do not review this PR yet.
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.