sequential bug when state is not given a new value

[leonardt]

@m.circuit.sequential(async_reset=True)
class Test:
    def __init__(self):
        self.x: m.Bits[8] = m.bits(0xFE, 8)

    def __call__(self, index: m.Bits[3]) -> m.Bit:
        return self.x[index]

gives the error

Traceback (most recent call last):
  File "/Users/lenny/repos/magma/magma/ast_utils.py", line 54, in compile_f
unction_to_file
    exec(code, defn_env)
  File ".magma/Test_comb.py", line 5, in <module>
    class Test_comb(m.Circuit):
  File "/Users/lenny/repos/magma/magma/circuit.py", line 471, in __new__
    self.definition()
  File ".magma/Test_comb.py", line 11, in definition
    __magma_ssa_return_value_0 = self_x_I, io.self_x_O[io.index]
NameError: name 'self_x_I' is not defined

it should set the default value of register input to be its output.

[leonardt]

Similarly, sub-sequential-circuits require a new value (call), both these would be resolved by implicitly generating "enable/valid" logic.

[leonardt]

This issue has been noted in the documentation: https://github.com/phanrahan/magma/blob/master/docs/circuit_definitions.md#sequential-circuit-definition

For optional updates of state elements, the algorithm seems straightforward (generate an enable signal that is set high when a write occurs (can default to be 0, with the input being the output value, rewrite the tree such that any write to a state element is followed by setting the enable signal high, optimize out the enable signal in the case that it is always high in every branch).

Optional invocations of sub-circuits is trickier. We can model is as an "implicit valid" on the input values, so a valid low would set all the sub-circuit's state elements to be low. This slightly changes the semantics that __call__ is invoked every cycle to be __call__ is invoked only with valid data (which is at most every cycle, and in this case, if valid is always true, then the valid logic can be removed). This extension could imply that we have an option to generate a sequential circuit with a valid input interface (or we could call it an enable).

[hofstee]

1. Why are you creating a register in this use-case? You're effectively making a LUT or a ROM, just make it combinational logic and let a synthesis tool create the circuit for it.

2. If you generate Verilog that is explicitly creating the enable signal with a mux in front of the register I am highly suspect that it will play nicely with synthesis tools. We already have problems where CoreIR clock-gated registers do not actually get implemented as clock-gated registers because it does a logical clock-gate with a mux in front of the register. This means with a clock-gated register with a write-enable, you get 2 muxes in front of the register and I doubt a synthesis tool would be happy with that.

3. Can you not just translate these things directly into Verilog? This seems straightforward to me, the above test would just be:

module Test(input  logic       clk, reset,
            input  logic [2:0] index,
            output logic       O);

  logic [7:0] x;

  always_ff @(posedge clk, posedge reset) begin
    if (reset) begin
      x <= 8'hFE;
    end
    else begin
    end
  end

  assign O = x[index];

endmodule

[leonardt]

Sequential semantics dictate that anything declared inside the __init__ is a register, if you wanted to avoid the register, you could simply write

@m.circuit.combinational
def test(self, index: m.Bits[3]) -> m.Bit:
        x = m.bits(0xFE, 8)
        return x[index]

I think the use case was that someone was iteratively developing their code and they just wanted to return the register value in the beginning, but this broke the compiler, sure this isn't something that someone would want to do, but it shouldn't break (it has well defined semantics).

w.r.t. generating the verilog directly versus going through coreir, I think that's a separate discussion/issue unrelated to this. The semantics should be the same, however it may be the case that the synthesis tool will interpret it differently depending on how the verilog is generated.

If you'd like to open an issue about getting better synthesis results from the magma generated code, please go ahead, code examples would be helpful for us to improve it.

This issue is just about fixing a bug in the compiler that prevents code from being compiled that should be able to be compiled (not about writing/generating optimal code).

[leonardt]

Re: compiling to verilog directly, there is an open PR related to this #441 you're welcome to contribute comments/code there

[leonardt]

Sorry, I misread number 2, I think this is in response to one of my follow up comments re: the enable logic for sub circuit invocations. Again, there should be some well defined semantics, but it seems like you're saying how we generate code may not play nicely with the synthesis tool. What is the best way to have enable logic with registers to play nicely with a synthesis tool?

[hofstee]

I think the most reliable way to do this would be to generate verilog similar to what I've written above except with if(condition) writing to the register and not explicitly computing the enable signal. This is how most humans would write the Verilog.

You may also get away with having a "register with enable" primitive that exposes D, Q, clk, w_en (or however you want to name them) and making sure that primitive is correctly inferred. Then you could probably compute the enable signal and wire it up.

I haven't looked into which of the two would give better synthesis results.