Skip to content
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

Closed
dleibrandt opened this issue Nov 17, 2016 · 8 comments
Closed
Assignees
Milestone

Comments

@dleibrandt
Copy link
Contributor

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 of set_pulse_time so that I can call it from both locations. Would it be possible to make seconds_to_mu on the core device accept the core argument, even if it doesn't use it?

class UnitConversionTime1(EnvExperiment):
    def build(self):
        self.setattr_device("core")
        
    def prepare(self):
        self.set_pulse_time(1.*us)
        
    @portable
    def set_pulse_time(self, t):
        self.t = t
        self.t_mu = seconds_to_mu(self.t, core=self.core)

    @kernel
    def run(self):
        self.set_pulse_time(1.*us)
@whitequark whitequark self-assigned this Nov 18, 2016
@sbourdeauducq
Copy link
Member

Alternatively seconds_to_mu and mu_to_seconds could be made methods of the core device object.

@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.

@whitequark
Copy link
Contributor

Alternatively seconds_to_mu and mu_to_seconds could be made methods of the core device object.

This seems preferable.

@dleibrandt
Copy link
Contributor Author

@sbourdeauducq 's suggestion sounds good to me.

I do see a significant speedup. A simplified example is below:

class UnitConversionTime3(EnvExperiment):
    def build(self):
        self.setattr_device("core")
        self.setattr_device("ttl12")

    def prepare(self):
        self.set_time(1000.*us)

    def set_time(self, t):
        self.t = t
        self.t_mu = seconds_to_mu(self.t, core=self.core)

    @kernel
    def set_time_kernel(self, t):
        self.t = t
        self.t_mu = seconds_to_mu(self.t)

    @kernel
    def run(self):
        for t in [20*us, 15*us, 10*us, 5*us, 2*us, 1*us]:
            print("t =", t, " us")
            self.set_time_kernel(t)

            self.core.reset()

            for _ in range(10000):
                #self.ttl12.pulse(self.t)
                self.ttl12.pulse_mu(self.t_mu)

With the pulse method commented and the pulse_mu method uncommented, RTIOUnderflow occurs when the pulse time is 1 us. With the pulse method uncommented and the pulse_mu method commented, RTIOUnderflow occurs when the pulse time is 20 us.

@jordens
Copy link
Member

jordens commented Nov 18, 2016

maybe also affected by #591

Sorry, something went wrong.

@sbourdeauducq sbourdeauducq added this to the 3.0 milestone Nov 20, 2016
@whitequark
Copy link
Contributor

@dleibrandt The primary problem in your case is that you haven't marked t as kernel invariant. Once this is done, the pulse and pulse_mu variants generate exactly identical code.

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?

Sorry, something went wrong.

@whitequark
Copy link
Contributor

whitequark commented Nov 20, 2016

@dleibrandt To help debug these issues, commit f5cca6b includes a pass that automatically detects attributes that may be marked kernel invariant:

conv3.py:30:33-30:39: note: attribute 't' of type 'artiq_run_conv3.UnitConversionTime3'
    is never written to; it could be marked as kernel invariant to potentially
    increase performance
                self.ttl1.pulse(self.t)
                                ^^^^^^

@sbourdeauducq
Copy link
Member

Kernel invariants won't actually help here, t is not one when set_time_kernel is used.

@whitequark
Copy link
Contributor

Titular issue fixed in 009d396; I am addressing the underlying reason for slowness in #624.

jordens added a commit that referenced this issue Nov 22, 2016
* 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).
  ...
jordens added a commit that referenced this issue Nov 22, 2016
* 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).
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants