-
Notifications
You must be signed in to change notification settings - Fork 59
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
|
Hello. You may have forgotten to update the changelog!
|
…d-new-oqd-pipeline
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2299 +/- ##
==========================================
- Coverage 97.31% 96.62% -0.69%
==========================================
Files 107 108 +1
Lines 12951 13066 +115
Branches 1075 1100 +25
==========================================
+ Hits 12603 12625 +22
- Misses 288 374 +86
- Partials 60 67 +7 ☔ 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.
| llvm_ir_path = os.path.join(str(circuit.workspace), f"{circuit_name}.ll") | ||
| with open(llvm_ir_path, "w", encoding="utf-8") as f: | ||
| f.write(llvm_ir_text) | ||
| print(f"LLVM IR file written to: {llvm_ir_path}") |
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.
Only print when verbose? Or should we always print?
| print(f"LLVM IR file written to: {llvm_ir_path}") | ||
|
|
||
| # Link to ARTIQ's binary | ||
| if output_elf_name is None: |
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.
Why do we need this condition? 🤔 Is it for the user to specify a path they want themselves?
| "-mtriple=armv7-unknown-linux-gnueabihf", | ||
| "-mcpu=cortex-a9", |
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.
Does this constaint the type of the host machine? i.e. does this mean an OQD script can only be run on these kinds of systems?
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.
So the function compile_to_artiq is solely compile to ARTIQ and this is the hardware that ARTIQ employed. Well, it might be chnaged if the OQD target to different FPGA board (except the ARTIQ).
| "-shared", | ||
| "--eh-frame-hdr", | ||
| "-m", | ||
| "armelf_linux_eabi", |
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.
Same here
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.
| if verbose: | ||
| print(f"[ARTIQ] Linking ELF: {' '.join(lld_args)}") | ||
|
|
||
| try: |
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.
This try-except block seems identical for llc and lld. Maybe we can factor them out into a helper function? But I'm also fine with it if you leave it as is 👍
|
|
||
| private: | ||
| /// Emit ARTIQ runtime wrapper for LLVM dialect kernel function | ||
| LogicalResult emitARTIQRuntimeForLLVMFunc(ModuleOp module, OpBuilder &builder, |
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.
module is a keyword in cpp, let's avoid it as a variable name maybe
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 for pointing out!
Co-authored-by: Paul <79805239+paul0403@users.noreply.github.com>
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.
| @@ -1,4 +1,4 @@ | |||
| # Copyright 2024 Xanadu Quantum Technologies Inc. | |||
| # Copyright 2024-2026 Xanadu Quantum Technologies Inc. | |||
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.
Why the copyright change? 😄
| @@ -1,4 +1,4 @@ | |||
| # Copyright 2024 Xanadu Quantum Technologies Inc. | |||
| # Copyright 2024-2026 Xanadu Quantum Technologies Inc. | |||
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.
Same here
| @@ -1,4 +1,4 @@ | |||
| # Copyright 2022-2023 Xanadu Quantum Technologies Inc. | |||
| # Copyright 2022-2026 Xanadu Quantum Technologies Inc. | |||
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.
same here
Co-authored-by: Mehrdad Malek <39844030+mehrdad2m@users.noreply.github.com>
Co-authored-by: Mehrdad Malek <39844030+mehrdad2m@users.noreply.github.com>
Co-authored-by: Mehrdad Malek <39844030+mehrdad2m@users.noreply.github.com>
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]