Discussion: syntax for immutable variables and attributes

[whitequark]

To me, it is clear that the only robust solution to #192, #193 and #236 is introducing some form of syntax that allows to describe immutable attribute and bindings. Let's discuss this here. @rjo @sbourdeauducq

The requirements for this syntax would be:

1. Ordinary Python syntax;
2. that does nothing when executed on the host interpreter (with the right import);
3. that clearly (for both humans and compilers) marks as immutable the corresponding local variable (for its entire scope, i.e. throughout the entire def statement the binding must have the same value) or attribute (for entire duration of a single experiment, the attribute must have the same value in all objects of the class it belongs to).

Let's think about both local variables and attributes for now. The syntax for attributes is an absolute requirement, whereas the syntax for local variables is a strong requirement but I will also consider working around that using some flow analysis.

[jordens]

(that rjo guy, that's not me, even though he has my first name and cool sun glasses ;-)

To clarify: There would not be much challenge in getting this (the parallel example code from all three issues) to work using transaction-style parallel (https://github.com/m-labs/artiq/issues/193#issuecomment-166022479) which is completely linear w.r.t. data and control flow. At least we should have that mechanism as a fallback for situations that can not be written in smart parallelism with constification. Its limitations are acceptable in the use cases that I can think of.

I am uncertain whether I can give much input on implementing the smart parallelism which would need the constification. Depending on the outcome of this discussion we might just want to abandon it.

[sbourdeauducq]

Any advantages of the transaction style over the dumb with parallel implementation that I proposed (https://github.com/m-labs/artiq/issues/193#issuecomment-172892165 point 1)?

[jordens]

A few: You get granularity and you can decide yourself when you seek back to entry (you don't need with sequential blocks and increasing indentation then). And you can use multiple and named managers simultaneously.

[whitequark]

Hm. Good news on this, actually. Yesterday I thought of a few new design approaches to this and we can ditch the constification of local variables; there is a combination of a flow analysis and a syntactic restriction that can achieve the same goal and should be straightforward to reason about. (As a bonus it's pretty easy to implement.)

The problem with mutable fields remains though.

[sbourdeauducq]

By "problem with mutable fields" you mean interleaving blocks that contain things like delay(self.something)?

Unfortunately, this could be a common use case, unless we change how the experiment argument system works. Users want configurable pulse times, set at experiment submission from the GUI. Those are typically set as attributes on the experiment object in its build method, and referenced later in the kernel as e.g. delay(self.pi_pulse_time).

It is also desirable to scan certain pulse times on the core device, with the scan parameters defined in the GUI. But for those cases, AFAICT the pulse sequences are simple enough that a non-interleaving solution (dumb parallel or transactional) would work just fine.

[whitequark]

By "problem with mutable fields" you mean interleaving blocks that contain things like delay(self.something)?

Yes.

Those are typically set as attributes on the experiment object in its build method, and referenced later in the kernel as e.g. delay(self.pi_pulse_time).

It would be enough if you could do something like self.setattr_immutable("pi_pulse_time"). It would be then forbidden to set the attribute in the compiled code, but no restrictions otherwise.

It is also desirable to scan certain pulse times on the core device, with the scan parameters defined in the GUI.

In principle, there's nothing impossible, since (assuming we do the immutable attribute thing) this would already possible for a "scan" done with for pulse_time in range(self.x):. The interleaver could be extended to have intrinsic knowledge of certain scans.

[sbourdeauducq]

I guess that the required intrinsic knowledge of the scan is at which point in the scan the code needs to be interleaved differently. Some types of scans go through points in random order, so this will not work.

Will the setattr_immutable play nice with an experiment object creating sub-objects (e.g. a "ion library" that contains higher-level functions) that do delays that would need to be interleaved?

[sbourdeauducq]

Those delays could be stored in the attributes of the sub-object, and their value depends on the attributes of the experiment object.

[whitequark]

You need to mark the attribute as immutable somehow. The compiler doesn't matter how or which objects in particular do it.

[sbourdeauducq]

Let's do this:

1. rename the current with parallel implementation to with interleave, without further modifications.
2. implement with parallel in the "dumb" way. Possibly this may be lowered to the transactional style that Robert is advocating, in which case both APIs would be exposed to the user. This is up to you @whitequark .
3. release ARTIQ 1.0. with interleave is an experimental developer-only feature in this release.
4. improve with interleave, using immutable attributes etc. and document it properly.

[whitequark]

I've implemented the dumb with parallel. It's lowered directly to ARTIQ IR.

@sbourdeauducq Can you update the documentation? I don't think I know the right way to describe it.

[sbourdeauducq]

Let's only mention with parallel in the documentation; with interleave is experimental/developer-only at this stage, and it is likely to change anyway.

[whitequark]

Yes, but I'm not sure how to best explain the limitations of with parallel: in relation to the problem domain.

[sbourdeauducq]

@whitequark Unit tests are failing.

[whitequark]

@sbourdeauducq Yes. Some tests are genuinely impossible to write using the new with parallel:. Consider test_rtt:

class RTT(EnvExperiment):
    def build(self):
        self.setattr_device("core")
        self.setattr_device("ttl_inout")

    @kernel
    def run(self):
        self.ttl_inout.output()
        delay(1*us)
        with parallel:
            # make sure not to send two commands into the same RTIO
            # channel with the same timestamp
            self.ttl_inout.gate_rising(5*us)
            with sequential:
                delay(1*us)
                t0 = now_mu()
                self.ttl_inout.pulse(1*us)
        self.set_dataset("rtt", mu_to_seconds(self.ttl_inout.timestamp_mu() - t0))

... or test_loopback_count:

    @kernel
    def run(self):
        self.loop_in.input()
        delay(1*us)
        with parallel:
            self.loop_in.gate_rising(2*us)
            with sequential:
                delay(1*us)
                t0 = now_mu()
                self.loop_out.pulse(1*us)
        self.set_dataset("rtt", mu_to_seconds(self.loop_in.timestamp_mu() - t0))

In both cases we use the same channel in different arms of the with parallel: statement, which of course is not possible.

[whitequark]

Hm, no, not in case of test_loopback_count. I will look closer at it now.

[sbourdeauducq]

We can probably remove test_rtt. It is a convenient form of test_loopback (!= test_loopback_count) that does not require any hardware (test_loopback needs you to physically connect loop_in and loop_out). But it has ugly bits to work around the "same channel" problem - the delay after self.ttl_inout.output(), and now, the requirement for with interleave...

In test_loopback, self.loop_in.input() and the delay can probably be removed.

[whitequark]

Ah sorry, I posted the source for test_loopback above, not testloopbackcount. The latter one is in fact impossible to run without interleaving too:

    @kernel
    def run(self):
        self.ttl_inout.output()
        delay(5*us)
        with parallel:
            self.ttl_inout.gate_rising(10*us)
            with sequential:
                for i in range(self.npulses):
                    delay(25*ns)
                    self.ttl_inout.pulse(25*ns)
        self.set_dataset("count", self.ttl_inout.count())

[sbourdeauducq]

I'm fine with either:

1. interleaving manually
2. using with interleave there
3. using two channels and requiring a physical loopback

[sbourdeauducq]

Those two problematic tests are not representative of real-world use, they just check that the RTIO stack is operating properly, in a convenient manner that does not require any other hardware than the board.

[whitequark]

We can do that with test_rtt but not test_loopback_count because our current interleaver is not smart enough to handle the latter.

[whitequark]

I mean, we can use with interleave for test_rtt.

[sbourdeauducq]

Then solution 3 everywhere (which means removing test_rtt that becomes redundant, and removing ttl_inout from the example DDB).

[whitequark]

AnalyzerTest also uses ttl_inout.

[whitequark]

I can port that to use loop_in/loop_out.

[sbourdeauducq]

Everything should be OK now, let's just run all the hardware unittests to verify.

[whitequark]

I've confirmed that with the following patch applied, ttl0 stops working either:

diff --git a/artiq/gateware/targets/kc705.py b/artiq/gateware/targets/kc705.py
index 16ad41b..9001977 100755
--- a/artiq/gateware/targets/kc705.py
+++ b/artiq/gateware/targets/kc705.py
@@ -226,7 +226,7 @@ class NIST_CLOCK(_NIST_Ions):

         rtio_channels = []
         for i in range(16):
-            if i % 4 == 3:
+            if i % 4 == 3 or True:
                 phy = ttl_serdes_7series.Inout_8X(platform.request("ttl", i))
                 self.submodules += phy
                 rtio_channels.append(rtio.Channel.from_phy(phy, ififo_depth=512))

[whitequark]

@sbourdeauducq Oh. I figured it out. You need at least a delay_mu(8) between self.ttl0.output() and self.ttl0.on(). If I add that everything works, and ttl3 is perfectly alive.

[jordens]

set_oe() and set_o() colliding should be a RTIOCollisionError.

[whitequark]

I verified that all related tests succeed.

[jordens]

Don't we want to reopen this (or a similar issue)? Const-ification of variables would also drastically decrease the attribute writeback traffic.

[whitequark]

https://github.com/m-labs/artiq/issues/322