Add EnableSignal, useful for making Instances compatible with EnableInserter

[nmigen-issue-migration]

Issue by Fatsie
Wednesday Dec 25, 2019 at 12:31 GMT
Originally opened as m-labs/nmigen#285

---

I am making wrappers around existing RTL code using Instance. These block have a clock input. This structure seems to not be compatible with EnableInserter. This issue is to see if there is a way to make it compatibl or alternatively at least have a warning or error when using EnableInserter.

Reduced example code is this:

#!/bin/env python3
from nmigen import (
    Signal, Elaboratable, Module, EnableInserter, ClockSignal, Instance
)
from nmigen.build import Platform
from nmigen.back.verilog import convert

class dff(Elaboratable):
    def __init__(self):
        self.d = Signal()
        self.q = Signal()

    def elaborate(self, platform):
        m = Module()

        m.submodules.inst = Instance(
            "DFF",
            i_clk=ClockSignal(), i_d=self.d, o_q=self.q,
        )

        return m

class dffce(Elaboratable):
    def __init__(self):
        self.ce = Signal()
        self._dff = _dff = dff()
        self.d = _dff.d
        self.q = _dff.q

    def elaborate(self, platform):
        m = Module()

        m.submodules.dff = EnableInserter(self.ce)(self._dff)

        return m


top = dffce()
top_code = convert(top, ports=[top.ce, top.d, top.q])
with open("top.v", "w") as f:
    f.write(top_code)

Giving the output:

/* Generated by Yosys 0.9+932 (git sha1 ff8529a, gcc 4.8.5 -fPIC -Os) */

(* generator = "nMigen" *)
(* \nmigen.hierarchy  = "top.dff" *)
module dff(d, q, clk);
  (* src = "/home/verhaegs/anaconda2/envs/nmigen/lib/python3.7/site-packages/nmigen/hdl/ir.py:538" *)
  input clk;
  (* src = "./ceinserter.py:10" *)
  input d;
  (* src = "./ceinserter.py:11" *)
  output q;
  DFF inst (
    .clk(clk),
    .d(d),
    .q(q)
  );
endmodule

(* generator = "nMigen" *)
(* top =  1  *)
(* \nmigen.hierarchy  = "top" *)
module top(d, q, clk, rst, ce);
  (* src = "./ceinserter.py:25" *)
  input ce;
  (* src = "/home/verhaegs/anaconda2/envs/nmigen/lib/python3.7/site-packages/nmigen/hdl/ir.py:538" *)
  input clk;
  (* src = "./ceinserter.py:10" *)
  input d;
  (* src = "./ceinserter.py:11" *)
  output q;
  (* src = "/home/verhaegs/anaconda2/envs/nmigen/lib/python3.7/site-packages/nmigen/hdl/ir.py:538" *)
  input rst;
  dff dff (
    .clk(clk),
    .d(d),
    .q(q)
  );

The ce signal is not used.

[nmigen-issue-migration]

Comment by whitequark
Wednesday Dec 25, 2019 at 12:38 GMT

---

Unfortunately it is not possible to write an equivalent of EnableInserter without it either (a) being able to understand the inner structure of the modules it mutates, or (b) inserting a vendor-specific clock gating primitive.

For (a), consider that it would have to know to replace a DFF with a DFFE and hook up the E input, or perhaps have to know that a DFF is a storage element and wire up a mux to the input. For (b), you could certainly do that (by injecting the dependency on the primitive through platform), but I find it very unlikely that you'll be happy with the output netlist, due to timing, routing, etc issues.

[nmigen-issue-migration]

Comment by whitequark
Wednesday Dec 25, 2019 at 12:39 GMT

---

Of course, if you are asking for an ability to customize the action of EnableInserter on your own Elaboratable, that's definitely technically feasible. It is fairly niche though and requires a fair bit of design work.

[nmigen-issue-migration]

Comment by Fatsie
Wednesday Dec 25, 2019 at 13:30 GMT

---

I do think the (b) option to have platform specific clock gating primitives is a good solution. One can then also error when the platform does not provide this feature and a domain ClockSignal() is used inside fragments from EnableInserter.
It's then the task of the platform to handle the ugliness; for ASICs I know clock gating is a standard function handled by the (proprietary) clock tree synthesis tools and static timing analysis. Support from yosys is likely needed to handle this properly.
In the mean time it would be good if user could be informed when support is lacking. It should not be silently ignored as is done in my example.

[nmigen-issue-migration]

Comment by whitequark
Wednesday Dec 25, 2019 at 13:35 GMT

---

I do think the (b) option to have platform specific clock gating primitives is a good solution.

I'm not convinced here at all. I think it's worth exploring, but I see many ways this can potentially go wrong, and in general, EnableInserter's usefulness primarily stems from its fully synchronous nature.

In the mean time it would be good if user could be informed when support is lacking. It should not be silently ignored as is done in my example.

I agree that emitting a diagnostic when EnableInserter encounters an Instance could work. Unfortunately the same cannot be said about ResetInserter; there is no option but to keep it silently skip any Instances, or you couldn't use Instances at all.

[nmigen-issue-migration]

Comment by Fatsie
Wednesday Dec 25, 2019 at 13:44 GMT

---

Couldn't you change ResetSignal() to get proper output taking into account ResetInserter() ?
Of course this likely assumes synchronous reset as otherwise the reset logic has to be glitch free.

[nmigen-issue-migration]

Comment by Fatsie
Wednesday Dec 25, 2019 at 13:47 GMT

---

And maybe also a EnableSignal() can be introduced next to ClockSignal() and it is then the task of the code that does use Instance to properly handle the EnableSignal().

[nmigen-issue-migration]

Comment by whitequark
Wednesday Dec 25, 2019 at 14:26 GMT

---

it is then the task of the code that does use Instance to properly handle the EnableSignal().

Ah I see, so your complaint isn't that the code that uses Instances, by default, doesn't behave correctly in presence of EnableInserter. The complaint is that currently it's not possible to make it behave correctly.

Yes, I think changing the semantics of ResetSignal and adding EnableSignal is reasonable. Though care needs to be taken to make sure that won't have unintentional consequences.

[nmigen-issue-migration]

Comment by Fatsie
Thursday Dec 26, 2019 at 11:25 GMT

---

Ah I see, so your complaint isn't that the code that uses Instances, by default, doesn't behave correctly in presence of EnableInserter.

What I want is that if a nMigen Elaboratable subobject uses Instance inside it still supports same features as pure nMigen code, including use of EnableInserter on the object. I prefer solution that does this by default but I can live with a solution where user of Instance has to include extra boilerplate to make is compatible with EnableInserter.
But in the latter case if the class that uses Instance does not include the extra boilerplate code people who use EnableInserter on the class may be confused and only find why it does not do what one wants to do after long debugging time.

[nmigen-issue-migration]

Comment by whitequark
Thursday Dec 26, 2019 at 11:32 GMT

---

I prefer solution that does this by default

nMigen has no idea which ports of the instance are clocks, resets, or enables, nor does it have any idea that it should e.g. turn DFFs into DFFEs. I don't understand how this feature can work without any additional effort spent while defining an Instance.

[nmigen-issue-migration]

Comment by Fatsie
Thursday Dec 26, 2019 at 12:13 GMT

---

nMigen does know that output of ClockSignal() is a clock though but from technical point it is easier to have extra EnableSignal() than trying to handle Enable from ClockSignal().

[nmigen-issue-migration]

Comment by whitequark
Thursday Dec 26, 2019 at 12:18 GMT

---

Sure, I'm fine with adding EnableSignal in principle, even though it's a bit of a strange addition. The problem can be better illustrated with a reset. You're concerned about a module using Instance not being reset with the clock domain. That's fair; but if you have (for example) an Instance of DFF, you simply cannot get it to reset. You have to use a DFFSR and wire the ResetSignal manually to it, which I would consider "extra boilerplate". I don't know of a solution that avoids this boilerplate.

[nmigen-issue-migration]

Comment by Fatsie
Thursday Dec 26, 2019 at 13:57 GMT

---

To me the DFF ignoring ResetInserter is similar to a Signal(reset_less=True). So signals ignoring reset is something people using nMigen are familiar with. This is not the case for synchronous code ignoring EnableInserter.

[nmigen-issue-migration]

Comment by whitequark
Thursday Dec 26, 2019 at 14:01 GMT

---

I see. Do I understand it correctly that you're suggesting that EnableInserter look for EnableSignal(), and if it can't find one, insert a clock gating primitive?

[nmigen-issue-migration]

Comment by Fatsie
Thursday Dec 26, 2019 at 14:06 GMT

---

I would only introduce EnableSignal() if you don't want to introduce a clock gating primitive, otherwise I would always have ClockSignal() insert a clock gating primitive if it is in a block on which EnableInserter() is called.
If technically feasible I would like it to be handled by the platform and not by the one using Instance.

[nmigen-issue-migration]

Comment by whitequark
Thursday Dec 26, 2019 at 14:29 GMT

---

There are a few issues here.

First, many FPGAs do not have a clock gating primitive. What should iCE40 do here, for example? There is no vendor-sanctioned way to gate a clock, and even worse, I don't think either the FOSS tools or the proprietary tools would be able to cope with the timing consequences of you abusing a LUT as one, even if you managed to find a truth table where it would not glitch.

Second, FPGAs that do have clock gating primitives typically have severe restrictions on their use (e.g.: you only get one per chip and nMigen already uses it by default, or there are placement restrictions, or there are timing restrictions, or they aren't glitchless, etc). So effectively this auto-gating feature for ClockSignal will be ASIC-only, since I cannot imagine any practical FPGA design where I would actually want to use it.

Third, I don't think your suggestion:

I would always have ClockSignal() insert a clock gating primitive if it is in a block on which EnableInserter() is called.

is viable even on ASICs. Consider that not every input of an Instance feeds synchronous logic inside that instance. It could be as well a combinatorial input--say the instance is for a clock mux, or simply an output buffer feeding a pad. There is no way to tell by looking at the Instance which inputs should be gated and which should not.