llvm / llvm/circt

[FIRRTL] Sometimes `cmem` unexpectedly generates no wmask port

Open
#6,351 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
2.2k
Forks
524
Avg merge
3d 2h
Merged PRs (30d)
46

Description

Hi,
If we write both sub-fields and whole entry of a `cmem`, the wmask ports will be missing.

## Correct result: only write sub-fields

```scala
FIRRTL version 3.1.0
circuit our :
module our :
input clock : Clock
input reset : UInt<1>
output io : { flip addr : UInt<3>[3], flip wen : UInt<1>, flip wdata : { s1 : UInt<2>, s2 : UInt<1>}, flip wenS1 : UInt<1>, flip wdataS1 : UInt<2>, rdata : { s1 : UInt<2>, s2 : UInt<1>}}

cmem data : { s1 : UInt<2>, s2 : UInt<1>} [8]
when io.wenS1 :
infer mport MPORT = data[io.addr[0]], clock
connect MPORT.s1, io.wdataS1
when io.wen :
infer mport MPORT_1 = data[io.addr[1]], clock
connect MPORT_1.s2, io.wdata.s2
infer mport io_rdata_MPORT = data[io.addr[2]], clock
connect io.rdata, io_rdata_MPORT
```
generates
```verilog
// Generated by CIRCT firtool-1.57.1

module data_8x3(
input [2:0] R0_addr,
input R0_en,
R0_clk,
output [2:0] R0_data,
input [2:0] W0_addr,
input W0_en,
W0_clk,
input [2:0] W0_data,
W0_mask,
W1_addr,
input W1_en,
W1_clk,
input [2:0] W1_data,
W1_mask
);

reg [2:0] Memory[0:7];
always @(posedge W0_clk) begin
if (W0_en & W0_mask[0])
Memory[W0_addr][32'h0 +: 1] <= W0_data[0];
if (W0_en & W0_mask[1])
Memory[W0_addr][32'h1 +: 1] <= W0_data[1];
if (W0_en & W0_mask[2])
Memory[W0_addr][32'h2 +: 1] <= W0_data[2];
if (W1_en & W1_mask[0])
Memory[W1_addr][32'h0 +: 1] <= W1_data[0];
if (W1_en & W1_mask[1])
Memory[W1_addr][32'h1 +: 1] <= W1_data[1];
if (W1_en & W1_mask[2])
Memory[W1_addr][32'h2 +: 1] <= W1_data[2];
end // always @(posedge)
`ifdef ENABLE_INITIAL_MEM_
reg [31:0] _RANDOM_MEM;
initial begin
`INIT_RANDOM_PROLOG_
`ifdef RANDOMIZE_MEM_INIT
for (logic [3:0] i = 4'h0; i < 4'h8; i += 4'h1) begin
_RANDOM_MEM = `RANDOM;
Memory[i[2:0]] = _RANDOM_MEM[2:0];
end
`endif // RANDOMIZE_MEM_INIT
end // initial
`endif // ENABLE_INITIAL_MEM_
assign R0_data = R0_en ? Memory[R0_addr] : 3'bx;
endmodule

module our(
input clock,
reset,
input [2:0] io_addr_0,
io_addr_1,
io_addr_2,
input io_wen,
input [1:0] io_wdata_s1,
input io_wdata_s2,
io_wenS1,
input [1:0] io_wdataS1,
output [1:0] io_rdata_s1,
output io_rdata_s2
);

wire [2:0] _data_ext_R0_data;
data_8x3 data_ext (
.R0_addr (io_addr_2),
.R0_en (1'h1),
.R0_clk (clock),
.R0_data (_data_ext_R0_data),
.W0_addr (io_addr_1),
.W0_en (io_wen),
.W0_clk (clock),
.W0_data ({io_wdata_s2, 2'h0}),
.W0_mask (3'h4),
.W1_addr (io_addr_0),
.W1_en (io_wenS1),
.W1_clk (clock),
.W1_data ({1'h0, io_wdataS1}),
.W1_mask (3'h3)
);
assign io_rdata_s1 = _data_ext_R0_data[1:0];
assign io_rdata_s2 = _data_ext_R0_data[2];
endmodule
```
which has wmask ports and seems okay.

## Incorrect result: write both sub-field and the whole entry

```scala
FIRRTL version 3.1.0
circuit our :
module our :
input clock : Clock
input reset : UInt<1>
output io : { flip addr : UInt<3>[3], flip wen : UInt<1>, flip wdata : { s1 : UInt<2>, s2 : UInt<1>}, flip wenS1 : UInt<1>, flip wdataS1 : UInt<2>, rdata : { s1 : UInt<2>, s2 : UInt<1>}}

cmem data : { s1 : UInt<2>, s2 : UInt<1>} [8]
when io.wenS1 :
infer mport MPORT = data[io.addr[0]], clock
connect MPORT.s1, io.wdataS1
when io.wen :
infer mport MPORT_1 = data[io.addr[1]], clock
connect MPORT_1, io.wdata
infer mport io_rdata_MPORT = data[io.addr[2]], clock
connect io.rdata, io_rdata_MPORT
```
generates
```verilog
// Generated by CIRCT firtool-1.57.1

module data_8x3(
input [2:0] R0_addr,
input R0_en,
R0_clk,
output [2:0] R0_data,
input [2:0] W0_addr,
input W0_en,
W0_clk,
input [2:0] W0_data,
W1_addr,
input W1_en,
W1_clk,
input [2:0] W1_data
);

reg [2:0] Memory[0:7];
always @(posedge W0_clk) begin
if (W0_en)
Memory[W0_addr] <= W0_data;
if (W1_en)
Memory[W1_addr] <= W1_data;
end // always @(posedge)
`ifdef ENABLE_INITIAL_MEM_
reg [31:0] _RANDOM_MEM;
initial begin
`INIT_RANDOM_PROLOG_
`ifdef RANDOMIZE_MEM_INIT
for (logic [3:0] i = 4'h0; i < 4'h8; i += 4'h1) begin
_RANDOM_MEM = `RANDOM;
Memory[i[2:0]] = _RANDOM_MEM[2:0];
end
`endif // RANDOMIZE_MEM_INIT
end // initial
`endif // ENABLE_INITIAL_MEM_
assign R0_data = R0_en ? Memory[R0_addr] : 3'bx;
endmodule

module our(
input clock,
reset,
input [2:0] io_addr_0,
io_addr_1,
io_addr_2,
input io_wen,
input [1:0] io_wdata_s1,
input io_wdata_s2,
io_wenS1,
input [1:0] io_wdataS1,
output [1:0] io_rdata_s1,
output io_rdata_s2
);

wire [2:0] _data_ext_R0_data;
data_8x3 data_ext (
.R0_addr (io_addr_2),
.R0_en (1'h1),
.R0_clk (clock),
.R0_data (_data_ext_R0_data),
.W0_addr (io_addr_1),
.W0_en (io_wen),
.W0_clk (clock),
.W0_data ({io_wdata_s2, io_wdata_s1}),
.W1_addr (io_addr_0),
.W1_en (io_wenS1),
.W1_clk (clock),
.W1_data ({1'h0, io_wdataS1})
);
assign io_rdata_s1 = _data_ext_R0_data[1:0];
assign io_rdata_s2 = _data_ext_R0_data[2];
endmodule
```
in which the wmask ports are missing.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Reproduce the two supplied FIRRTL examples with firtool 1.57.1 and compare their generated Verilog memory interfaces. Trace the FIRRTL memory-lowering path for mixed whole-entry and sub-field writes; done means the mixed case retains wmask ports and preserves the behavior shown by the sub-field-only case.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.