-
Notifications
You must be signed in to change notification settings - Fork 201
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
seconds_to_mu should take the same in prepare and on the core device #622
Comments
Alternatively @dleibrandt If you notice any performance improvement with this technique, please post your code as I suspect that this is something that the compiler should have done itself e.g. with inlining/constant propagation. |
This seems preferable. |
@sbourdeauducq 's suggestion sounds good to me. I do see a significant speedup. A simplified example is below:
With the |
maybe also affected by #591 |
@dleibrandt The primary problem in your case is that you haven't marked If you are pre-calculating values such as timings, it is best to pass them through arguments. Storing them in attributes inside the kernel will hinder the optimizer. Do you specifically want to store such calculated values in attributes? Can I see a more realistic example so that I can try to make it more ergonomic, within the constraints of the optimizer? |
@dleibrandt To help debug these issues, commit f5cca6b includes a pass that automatically detects attributes that may be marked kernel invariant:
|
Kernel invariants won't actually help here, |
* master: (23 commits) RELEASE_NOTES: update pipistrello: add some inputs Remove last vestiges of nist_qc1. Fully drop AD9858 and kc705-nist_qc1 support (closes #576). coredevice.dds: reimplement fully in ARTIQ Python. compiler: unbreak casts to int32/int64. analyses.constness: fix false positive on x[...]. inferencer: significantly improve the op-assignment diagnostic. Fix tests. Move mu_to_seconds, seconds_to_mu to Core. artiq_devtool: don't crash on invalid utf-8. artiq_devtool: detect a race condition during connect. llvm_ir_generator: handle no-op coercions. conda: use development version of migen/misoc Revert accidentally committed code. Revert "gateware: increase RTIO FIFO sizes for NIST_CLOCK. Closes #623" analyses.invariant_detection: implement (#622). Fix whitespace. coredevice.dds: work around the round(numpy.float64()) snafu. coredevice.dds: update from obsolete int(width=) syntax (fixes #621). ...
* phaser: (23 commits) RELEASE_NOTES: update pipistrello: add some inputs Remove last vestiges of nist_qc1. Fully drop AD9858 and kc705-nist_qc1 support (closes #576). coredevice.dds: reimplement fully in ARTIQ Python. compiler: unbreak casts to int32/int64. analyses.constness: fix false positive on x[...]. inferencer: significantly improve the op-assignment diagnostic. Fix tests. Move mu_to_seconds, seconds_to_mu to Core. artiq_devtool: don't crash on invalid utf-8. artiq_devtool: detect a race condition during connect. llvm_ir_generator: handle no-op coercions. conda: use development version of migen/misoc Revert accidentally committed code. Revert "gateware: increase RTIO FIFO sizes for NIST_CLOCK. Closes #623" analyses.invariant_detection: implement (#622). Fix whitespace. coredevice.dds: work around the round(numpy.float64()) snafu. coredevice.dds: update from obsolete int(width=) syntax (fixes #621). ...
The following code throws an error, because
seconds_to_mu
requires different arguments in prepare and on the core device. It's annoying to write two versions ofset_pulse_time
so that I can call it from both locations. Would it be possible to makeseconds_to_mu
on the core device accept thecore
argument, even if it doesn't use it?The text was updated successfully, but these errors were encountered: