Switching to Non-Blocking Assigns in Single/Dual Port Ram Primitives

[loglav03]

Description

Switched read and write updates to use non-blocking assigns instead of blocking assigns for single/dual port ram primitives.

Related Issue

#3195

Motivation and Context

This change is needed to resolve issue with block RAMs being expanded into LUTs and FFs in some cases when parsing with Slang.

How Has This Been Tested?

Tested locally with cloned version of vtr_reg_qor test that uses modified benchmarks from #3195 and uses Yosys-Slang as the parser. Most of these benchmarks pass the flow but still fail golden results, which is expected.

Also locally tested vtr_reg_qor with old version of benchmarks using default yosys parser. [PASSED]

Passes all existing CI tests.

Types of changes

• [ x] Bug fix (change which fixes an issue)
• New feature (change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My change requires a change to the documentation
• I have updated the documentation accordingly
• I have added tests to cover my changes
• [ x] All new and existing tests passed

QoR Comparison Results (Using Default Yosys Parser, comparing write-first vs. read-first):
Comparison Tables: compare_tables.zip
Summary of Tables: summary.xlsx

vtr_reg_qor_chain:

• vtr_flow_elapsed_time, parmys_synth_time, abc_synth_time, pack_time, place_time, min_chan_width_route_time, and crit_path_route_time all had 5% - 7% decrease (gain).
• The rest (including crit_path_delay) have minimal or no change. crit_path_delay has no change
• From looking at various benchmarks in this test comparison, found no change in the number of FFs.

koios_medium:

• parmys_synth_time: 5% increase (loss)
• abc_synth_time: 3% increase (loss)
• place_time: 4% decrease (gain)
• crit_path_route_time: 3% decrease (gain)
• The rest have minimal or no change. crit_path_delay has no change
• Once again, found no change in number of FFs.

[petergrossmann21]

This is the correct change with respect to synthesis; are there any HDL simulation tests that need to be checked to ensure that our behavior change is not breaking?

[loglav03]

This is the correct change with respect to synthesis; are there any HDL simulation tests that need to be checked to ensure that our behavior change is not breaking?

I'm honestly not sure. It's probably best for either @AlexandreSinger or @vaughnbetz to answer this.

[AlexandreSinger]

@loglav03 I am ok with this change, I understand why you are doing this. My concern is that we are implicitly changing the behavior of the RAMs. We are changing the RAMs from a write-first behavior to a read-first behavior. I am honestly not sure what affect this has... I guess we would need to know the context as to why write-first behavior was chosen in the first place. The reason I have concern is that the author of this file explicitly wrote that they wanted "read-during write behaviour"; so I am wondering if that was a coin-flip choice or not.

@vaughnbetz I think you may have the best judgement on this one. I do not think this would affect pack place and route all that much, but I am not sure if VTR follows a standard for RAM R/W behaviors?

@petergrossmann21 and @loglav03 do we need to turn this into read-first behavior, or does slang support write-first RAM inference?

[loglav03]

@petergrossmann21 and @loglav03 do we need to turn this into read-first behavior, or does slang support write-first RAM inference?

I believe slang doesn't forbid write-first, but when using blocking assignments with slang as the parser, the RTL it passes to yosys doesn't match the pattern yosys's memory inference pass would use to infer RAM. However, when using the default yosys parser it doesn't seem to have a problem with this. So, it seems it's parser dependent when it comes to using write-first with blocking assignments.

[petergrossmann21]

@AlexandreSinger Slang is just a parser, which is too early in the synthesis flow to dictate whether read-first/write-first support is present or not. The change to the flow is that RAM inference is now required at all, vs. in the original flow the memories were more or less blackboxed. This does raise the question as to how the memories are getting back into a technology-mapped form (we know they are, but it's a bit opaque as to how) -- I assume parmys is taking care of this?

[loglav03]

@petergrossmann21 - It does appear parmys takes care of this. I found the following functions that occur at some point during the parmys elaboration pass: resolve_single_port_ram -> calls create_single_port_ram. These functions hint towards this. These can be found in vtr-root/parmys/parmys-plugin/core/memory.cc at line 1990 or by following the resolve_top call in parmys.cc at line 749

And these only appear after the parmys pass (which I assume are the memories in tech-mapped form):

single_port_ram _4855_ (
    .addr({ _4839_, _4839_, _4839_, _4839_, _4839_, _4839_, _4839_, _0897_, _0896_, _0895_, _0894_, _0893_, _0892_, _0891_, _0890_ }),
    .clk(clk),
    .data(\rgCfgData~0 ),
    .out(\single_port_ram^MEM~7-0^out~0 ),
    .we(_0671_)
  );
  single_port_ram _4856_ (
    .addr({ _4839_, _4839_, _4839_, _4839_, _4839_, _4839_, _4839_, _0897_, _0896_, _0895_, _0894_, _0893_, _0892_, _0891_, _0890_ }),
    .clk(clk),
    .data(\rgCfgData~1 ),
    .out(\single_port_ram^MEM~7-1^out~0 ),
    .we(_0671_)
  );
....

[vaughnbetz]

As we discussed in the meeting, we can make this change:

• on the original designs, if the FF count and cpd don't change much or
• On a new directory of vtr_sv/ designs, if the change impacts result quality significantly. CI tests for sv will use the new directory in this case, while older tests and comparisons to papers will use the original vtr/ directory designs.

[loglav03]

@vaughnbetz @AlexandreSinger @petergrossmann21 - I've attached a QoR comparison result section to the description of this PR, that compares metrics when using write-first vs read-first behaviour with the default yosys parser. This seemed to generally yield gains in flow times, and have minimal or no difference in other metrics. I think this change can be kept across the board since it has minimal metric differences with original designs using the default yosys parser, and fixes the issue we've encountered when using the slang parser on modified designs.

[vaughnbetz]

@loglav03 : I think you forgot to attach the QoR comparison?

[loglav03]

@vaughnbetz It should be at the bottom of the PR description below the checklist section? It includes a .zip of the QoR comparison tables of vtr_reg_qor_chain and koios_medium. I'm also going to include a single summary spreadsheet for both comparisons.

[vaughnbetz]

Looks good to me, thanks!