-
Notifications
You must be signed in to change notification settings - Fork 59
Lower RTIO to LLVM IR #2273
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: rniczh/lower-ion-to-rtio
Are you sure you want to change the base?
Lower RTIO to LLVM IR #2273
Conversation
…alyst into rniczh/convert-rtio-to-llvmir
|
Hello. You may have forgotten to update the changelog!
|
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.
Thank @rniczh, Wow, this looks amazing! I'll add some minor comments gradually. I am also approving as I will be away from tomorrow to unblock things.
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.
Note that at the time of the review, there is no tests added. We have to add tests to check the generated IR
…alyst into rniczh/convert-rtio-to-llvmir
…I/catalyst into rniczh/convert-rtio-to-llvmir
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## rniczh/lower-ion-to-rtio #2273 +/- ##
============================================================
- Coverage 97.21% 97.19% -0.02%
============================================================
Files 93 93
Lines 11243 11248 +5
Branches 1075 1078 +3
============================================================
+ Hits 10930 10933 +3
- Misses 246 247 +1
- Partials 67 68 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Really amazing! Thanks Hongsheng! 💯
Context:
Description of the Change:
The conversion from RTIO to LLVM IR can be divided into three steps:
Scheduling
The purpose of schedule step is to further distinguish which regions must be executed serially based on HW constraints derived from ARTIQ. Here we employ BFS to identify the first feasible group and repeat it with the remaining RTIO operation to find all feasible groups.
Currently, a major constraint is that events sharing the same channel must have a unique frequency setting. In other words, if a group contains the same channel but with different frequencies, it’s considered an illegal group.
Frequency Setting
For every feasible group, we execute their frequency setting serially before the group itself starts to run. This ensures that the corresponding channel has been updated to the correct frequency via SPI register and I/O updates on the device before the group execution begins. The IR graph shown here illustrates this intermediate state. Normally, the process lowers directly to LLVM IR, so this step represents a transient state within the IR, which will not appear in the final output of the pass, since the result should already be in LLVM IR.
And there is only one SPI bus available at the device. So each time you can only update one frequency. We also adopt a simple optimization: If that channel has already been updated to a certain frequency then we don’t need to do the same frequency setting again.
Lowering to LLVM IR
Converting from RTIO to LLVM IR is straight forward:
rtio.emptyJust call
now_mu()to get the current timestamp of ARTIQ and return itrtio.pulseInvolve three steps in
rtio.pulse, we split each pulse to three steps during the frequency setting part.Because frequency setting takes time, it involves the setting from CPU to update value to SPI register, needing to do the delay on the cursor at the same time to ensure no underflow issue happens on the device.
step 1 + 2:
step 3:
rtio.syncWe sync the events by collecting their timestamps and find the maximum value from them and jump to there. And transfer the timestamp through
now_mu()as wellBenefits:
Possible Drawbacks:
Related GitHub Issues: