[FIRRTL] Return multiple results from memories, Lower Memories in LowerTypes

[seldridge]

Fixes #479, #478.

Return one result per port for a FIRRTL memory instead of a single
bundle.

Lowers memories during FIRRTL type lowering.

Status

This is still work-in-progress and there's a lot of cargo culting from ded5bd2 and 938ad62. This also has some very ugly code in LowerTypes that I'll cleanup before promoting to non-draft.

[lattner]

Nice. I'm really happy you tackled this, thank you. I just did a quick scan and this is going in the right direction, lemme know when you want a detailed review thx!

[seldridge]

Note that this is not ready for review yet. The memory type lowering is very ugly right now and should be cleaned up before anybody looks at it.

That said, the output here is looking pretty good through the FIRRTL type lowering path.

Compiling with:

firtool -lower-to-rtl -enable-lower-types -verilog Bar.fir 

Input FIRRTL IR

circuit Bar :
  module Bar :
    input clock: Clock
    input rAddr: UInt<4>
    input rEn: UInt<1>
    output rData: {a: UInt<8>, b: UInt<8>}
    input wAddr: UInt<4>
    input wEn: UInt<1>
    input wMask: {a: UInt<1>, b: UInt<1>}
    input wData: {a: UInt<8>, b: UInt<8>}

    mem memory:
      data-type => {a: UInt<8>, b: UInt<8>}
      depth => 16
      reader => r
      writer => w
      read-latency => 0
      write-latency => 1
      read-under-write => undefined

    memory.r.clk <= clock
    memory.r.en <= rEn
    memory.r.addr <= rAddr
    rData <= memory.r.data

    memory.w.clk <= clock
    memory.w.en <= wEn
    memory.w.addr <= wAddr
    memory.w.mask <= wMask
    memory.w.data <= wData

Output Veriog

module Bar(
  input        clock,
  input  [3:0] rAddr,
  input        rEn,
  input  [3:0] wAddr,
  input        wEn, wMask_a, wMask_b,
  input  [7:0] wData_a, wData_b,
  output [7:0] rData_a, rData_b);

  reg  [7:0] memory_a[15:0];	// Bar.fir:12:5
  wire [3:0] memory_a_r_addr;	// Bar.fir:12:5
  wire [7:0] memory_a_r_data;	// Bar.fir:12:5
  wire [3:0] memory_a_w_addr;	// Bar.fir:12:5
  wire       memory_a_w_en;	// Bar.fir:12:5
  wire       memory_a_w_clk;	// Bar.fir:12:5
  wire [7:0] memory_a_w_data;	// Bar.fir:12:5
  wire       memory_a_w_mask;	// Bar.fir:12:5
  wire [3:0] memory_r_addr;	// Bar.fir:12:5
  wire [3:0] memory_w_addr;	// Bar.fir:12:5
  wire       memory_w_en;	// Bar.fir:12:5
  wire       memory_w_clk;	// Bar.fir:12:5
  reg  [7:0] memory_b[15:0];	// Bar.fir:12:5
  wire [3:0] memory_b_r_addr;	// Bar.fir:12:5
  wire [7:0] memory_b_r_data;	// Bar.fir:12:5
  wire [3:0] memory_b_w_addr;	// Bar.fir:12:5
  wire       memory_b_w_en;	// Bar.fir:12:5
  wire       memory_b_w_clk;	// Bar.fir:12:5
  wire [7:0] memory_b_w_data;	// Bar.fir:12:5
  wire       memory_b_w_mask;	// Bar.fir:12:5

  `ifndef SYNTHESIS	// Bar.fir:12:5
    initial begin	// Bar.fir:12:5
      `INIT_RANDOM_PROLOG_	// Bar.fir:12:5
      `ifdef RANDOMIZE_MEM_INIT	// Bar.fir:12:5
        integer memory_a_initvar;
        for (memory_a_initvar = 0; memory_a_initvar < 16; memory_a_initvar = memory_a_initvar+1)
          memory_a[memory_a_initvar] = `RANDOM;	// Bar.fir:12:5
      `endif
    end // initial
  `endif
  assign memory_a_r_data = memory_a[memory_a_r_addr];	// Bar.fir:12:5
  always @(posedge memory_a_w_clk) begin	// Bar.fir:12:5
    if (memory_a_w_en & memory_a_w_mask) begin	// Bar.fir:12:5
      memory_a[memory_a_w_addr] = memory_a_w_data;	// Bar.fir:12:5
    end
  end // always @(posedge)
  wire [3:0] _T = memory_r_addr;	// Bar.fir:12:5
  assign memory_a_r_addr = _T;	// Bar.fir:12:5
  wire [3:0] _T_0 = memory_w_addr;	// Bar.fir:12:5
  assign memory_a_w_addr = _T_0;	// Bar.fir:12:5
  wire _T_1 = memory_w_en;	// Bar.fir:12:5
  assign memory_a_w_en = _T_1;	// Bar.fir:12:5
  wire _T_2 = memory_w_clk;	// Bar.fir:12:5
  assign memory_a_w_clk = _T_2;	// Bar.fir:12:5
  `ifndef SYNTHESIS	// Bar.fir:12:5
    initial begin	// Bar.fir:12:5
      `ifdef RANDOMIZE_MEM_INIT	// Bar.fir:12:5
        integer memory_b_initvar;
        for (memory_b_initvar = 0; memory_b_initvar < 16; memory_b_initvar = memory_b_initvar+1)
          memory_b[memory_b_initvar] = `RANDOM;	// Bar.fir:12:5
      `endif
    end // initial
  `endif
  assign memory_b_r_data = memory_b[memory_b_r_addr];	// Bar.fir:12:5
  always @(posedge memory_b_w_clk) begin	// Bar.fir:12:5
    if (memory_b_w_en & memory_b_w_mask) begin	// Bar.fir:12:5
      memory_b[memory_b_w_addr] = memory_b_w_data;	// Bar.fir:12:5
    end
  end // always @(posedge)
  assign memory_b_r_addr = _T;	// Bar.fir:12:5
  assign memory_b_w_addr = _T_0;	// Bar.fir:12:5
  assign memory_b_w_en = _T_1;	// Bar.fir:12:5
  assign memory_b_w_clk = _T_2;	// Bar.fir:12:5
  assign memory_r_addr = rAddr;	// Bar.fir:23:19
  assign memory_w_clk = clock;	// Bar.fir:26:18
  assign memory_w_en = wEn;	// Bar.fir:27:17
  assign memory_w_addr = wAddr;	// Bar.fir:28:19
  assign memory_a_w_mask = wMask_a;	// Bar.fir:29:19
  assign memory_b_w_mask = wMask_b;	// Bar.fir:29:19
  assign memory_a_w_data = wData_a;	// Bar.fir:30:19
  assign memory_b_w_data = wData_b;	// Bar.fir:30:19
  assign rData_a = memory_a_r_data;	// Bar.fir:2:3
  assign rData_b = memory_b_r_data;	// Bar.fir:2:3
endmodule

[seldridge]

This is good for a review.

[mikeurbach]

I reviewed each commit here separately. In general, I think the logic and test cases are correct.

I think the first commit is ready to land as-is.

I had some concerns about the style in the second commit, but I'm not an authority on LLVM/C++ style.

[seldridge]

Annoyingly, the first commit, applied by itself, will cause LowerTypes to start crashing. If the second commit is problematic, I can patch the first so that it can land (and unblock #493).

My only big concern with the second commit is the duplicate walking of the both the old memory and the new memory. I can think of a way to simplify that or can modify that in a follow-up commit.

[youngar]

I was using (the slower) SubfieldOp::getResultType(op.getResult(i), field.name, op.getLoc()) to get this done. I'm not totally sure what I'm doing. Maybe we should factor this out into a helper that can get the field type by index (it could work for vectors?)?

[seldridge]

@lattner: Can you take a look at this when you get a chance?

[lattner]

Looks great!

[mikeurbach]

@seldridge Just an FYI, I checked out this branch and tried lowering a pretty complex piece of FIRRTL from Handshake using the changes in LowerTypes. It works great! I am not promising it is logically correct, but it unblocks the lowering all the way to (System) Verilog, as expected from the discussion here: #337 (comment).

This example program was complex enough to reveal another issue in LowerTypes, which I'm logging, reducing a test case for, and will have a PR for today: #534. I am only touching the visitor for connects, so I don't forsee any conflicts with what you're doing here.

[seldridge]

Slight issue with this and f736bc5. I'm looking into it now...

[lattner]

Ok thanks. when this lands again, plz update the FIRRTL rationale to mention this as well, thanks!