-
Notifications
You must be signed in to change notification settings - Fork 63
New OQD end-to-end pipeline is added to Catalyst #2299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…d-new-oqd-pipeline
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2299 +/- ##
==========================================
- Coverage 97.35% 96.96% -0.40%
==========================================
Files 107 108 +1
Lines 13136 13223 +87
Branches 1074 1089 +15
==========================================
+ Hits 12789 12822 +33
- Misses 286 339 +53
- Partials 61 62 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
paul0403
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Hongsheng, looks really nice! I think this is the minimally intrusive way we can do it without altering too much of the base jit flow!
I left some comments but all are minor. Another thing is can you add your PR description's example as an end-to-end pytest in the frontend oqd tests? Obviously no need for execution, just check that the compiled ELF exist (maybe check a little bit of its content as well)? You may need to do things like subprocess.run("which llc", shell=True) in the test, but I think that's possible.
Co-authored-by: Paul <[email protected]>
mehrdad2m
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rniczh, I did a first round of review with some minor comments. However, I would like to play around with a bit more and maybe more review on Monday.
Co-authored-by: Mehrdad Malek <[email protected]>
Co-authored-by: Mehrdad Malek <[email protected]>
dime10
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright I'll approve the qjit updates (note I haven't reviewed the OQD portions) because the changes are kept fairly small, however there is change in contract / behaviour of QJIT class in the following ways:
compileis no longer guaranteed to output aCompiledFunction(wrapping binary code) which it was guaranteed to do until now__call__/jit_compilewill no longer force compilation of the function down to binary if the target is set as"llvmir", as a resultrunwill raise an error for this case- this is different than the other target levels (
"jaxpr","mlir") which only control aot compilation, while calling the function will force the full compilation pipeline so the program can be executed - once the "llvmir" target is set there is no way to compile the program for execution, except by mutating the compile options in place or defining a new qjit object
- this is different than the other target levels (
The inconsistency could be solved by limiting the change (e.g. the internal link option) to take effect in AOT mode only. Alternatively, one could expand the target option to affect both AOT and and JIT mode for all targets, but I still think this comes with downsides (see the last bullet point above).
However, the change in the contract structure of QJIT (main idea: QJIT is broken down into discrete stages, each of which produces well defined results that can feed into the next stage) might only be resolvable by breaking the compile step up such that we have separate compile and link stages.
If @mehrdad2m is happy with the current state we can merge this PR but I think these two points will be added as technical debt.
Oh I thought we agreed on keeping things as before for this PR as there is no consensus on the ideal behaviour. This is a very user facing feature and not also very related to this PR, so even if we want to change anything, I think it's best to get in sync with product team first and do it in another PR. @rniczh If you don't mind, let's revert the change that raises error for |
|
@mehrdad2m Sure, I reverted it in this commit: 67dba09 |
| else: | ||
| shared_object, llvm_ir = self.compiler.run(self.mlir_module, self.workspace) | ||
|
|
||
| if not self.compile_options.link: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this still crash the jit_compile path if we have target=llvmir? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, should remove those setting as well. So the current goal is to remove all LLVM IR specializations in the pipeline. For now, we're reverting to only adding the link setting. Since end-to-end is not possible without disabling link, when using OQD's end-to-end, we need to change to actively construct using QJIT.
Changed: 9d80502
Therefore, in terms of usage, we're changing the approach to first create a qnode with @qnode, then use QJIT with the current compile options to achieve this.
oqd_pipelines = _get_oqd_pipelines()
@qml.set_shots(4)
@qml.qnode(oqd_dev)
def circuit():
x = np.pi / 2
qml.RX(x, wires=0)
return qml.counts(all_outcomes=True)
# Get the LLVM IR
compiled_circuit = QJIT(circuit, CompileOptions(link=False, pipelines=oqd_pipelines))
llvm_ir = compiled_circuit.llvmirThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Did we remove a test for |
Context:
Description of the Change:
This pipeline enables Catalyst to compile circuit for ARTIQ-based quantum device. When
device_dbconfiguration is provided, the compilation follows the ARTIQ route:ions-decomposition: Decompose quantum operations for trapped ion systemsgates-to-pulses: Convert gate operations to pulse sequencesconvert-ion-to-rtio: Convert ion operations to RTIO dialectconvert-rtio-event-to-artiq: Lower RTIO event to ARTIQ's primitivesllvm-dialect-lowering-stage: Lower to LLVM IRemit-artiq-runtime: Generate ARTIQ runtime entry point as wrapper for ARITQ device to executeThe final stage compiles LLVM IR to an ELF binary targeting the ARTIQ device (ARM Cortex-A9). Due to current limitations where Catalyst's internal LLVM build does not include the corresponding ARM backend, the compilation will use
llcto compile.llto object file, and useld.lldto compile to.elfWe added a compile_to_artiq helper function to the oqd module, so it can compile and link Catalyst-generated LLVM IR to ARTIQ's binary, keep OQD-specific logic out of Catalyst core.
Example:
Replace the artiq configs to your own env setting:
The result will store to
circuit.elfAnd you can run it on artiq device:
Benefits:
Possible Drawbacks:
It causes the OQD specific compile stuffs steps into the original catalyst compiler driver's pipeplineRelated GitHub Issues:
[sc-100853]