Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HWMemSimImpl] Fix RW port enable gating #5700

Merged
merged 1 commit into from
Jul 27, 2023
Merged

Conversation

nandor
Copy link
Contributor

@nandor nandor commented Jul 27, 2023

No description provided.

@nandor nandor requested review from seldridge and uenoku July 27, 2023 14:52
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a test. 😉

llvm Outdated Show resolved Hide resolved
Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nandor nandor merged commit 636d4a4 into main Jul 27, 2023
5 checks passed
@nandor nandor deleted the dev/nandor/fix-rw-ports branch July 27, 2023 15:50
@seldridge
Copy link
Member

The behavior here for a read-write port is not exactly the same as that for a read port. The output with this option (with this patch) is:

  reg [31:0] Memory[0:7];
  reg [2:0]  _GEN;
  always @(posedge R0_clk) begin
    if (R0_en)
      _GEN <= R0_addr;
  end // always @(posedge)
  reg [2:0]  _GEN_0;
  reg        _GEN_1;
  reg        _GEN_2;
  always @(posedge RW0_clk) begin
    _GEN_0 <= RW0_addr;
    _GEN_1 <= RW0_en;
    _GEN_2 <= RW0_wmode;
    if (RW0_en & RW0_wmode)
      Memory[RW0_addr] <= RW0_wdata;
  end // always @(posedge)
  assign R0_data = Memory[_GEN];
  assign RW0_rdata = Memory[_GEN_0];

The behavior here should be the same. I.e., this should return either the old address or just ignore the read enable. I'm inclined for the former as it still preserves the functionality of the read enable while the latter does not.

WDYT?

@seldridge
Copy link
Member

I checked the old SFC behavior for this and this produces the following output:

  reg [31:0] memory [0:7];
  wire  memory_r_en;
  wire [2:0] memory_r_addr;
  wire [31:0] memory_r_data;
  wire  memory_rw_r_en;
  wire [2:0] memory_rw_r_addr;
  wire [31:0] memory_rw_r_data;
  wire [31:0] memory_rw_w_data;
  wire [2:0] memory_rw_w_addr;
  wire  memory_rw_w_mask;
  wire  memory_rw_w_en;
  reg  memory_r_en_pipe_0;
  reg [2:0] memory_r_addr_pipe_0;
  reg  memory_rw_r_en_pipe_0;
  reg [2:0] memory_rw_r_addr_pipe_0;
  assign memory_r_en = memory_r_en_pipe_0;
  assign memory_r_addr = memory_r_addr_pipe_0;
  assign memory_r_data = memory[memory_r_addr];
  assign memory_rw_r_en = memory_rw_r_en_pipe_0;
  assign memory_rw_r_addr = memory_rw_r_addr_pipe_0;
  assign memory_rw_r_data = memory[memory_rw_r_addr];
  assign memory_rw_w_data = rw_wdata;
  assign memory_rw_w_addr = rw_addr;
  assign memory_rw_w_mask = rw_wmask;
  assign memory_rw_w_en = rw_en & rw_wmode;
  assign r_data = memory_r_data;
  assign rw_rdata = memory_rw_r_data;
  always @(posedge rw_clk) begin
    if (memory_rw_w_en & memory_rw_w_mask) begin
      memory[memory_rw_w_addr] <= memory_rw_w_data;
    end
    memory_rw_r_en_pipe_0 <= rw_en & ~rw_wmode;
    if (rw_en & ~rw_wmode) begin
      memory_rw_r_addr_pipe_0 <= rw_addr;
    end
  end
  always @(posedge r_clk) begin
    memory_r_en_pipe_0 <= r_en;
    if (r_en) begin
      memory_r_addr_pipe_0 <= r_addr;
    end
  end

We should update this to do the same thing here.

@seldridge
Copy link
Member

Fix to align things is here: #5704

seldridge added a commit that referenced this pull request Jul 27, 2023
Fix an inconsistency between the handling of read ports and read-write
ports when running HWMemSimImpl with the "ignore-read-enable"
option (firtool option "-ignore-read-enable-mem").  After #5700, this
would cause the enable line of a read-write read port to have no effect.
I.e., the memory would always read even if not enabled.  Change this to
align with the behavior of read ports where the memory always returns the
last read address.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants