Verilator-compatible testbench, phase 2/2

[steveri]

I am changing our garnet testbench to be verilator-compatible, meaning that it can successfully run a single regression test to completion (pointwise). This pull request is phase two of two, completing the changes.

To make the code work with verilator, I did the following

• remove unrecognized "clocking" structures from interface definitions;
• rewrite test bench code to use parameterless global tasks instead of classes; this has the added benefit that global signals are more easily traced when /using third-party tools such as gtkwave;

In addition, I changed how the testbench deals with interrupts from the CGRA and global buffer in a way that, I think, simplifies the code and makes it more robust. Also, it eliminates some unnecessary forking that had previously been required. To see the relevant changes, look in the code for Env_wait_interrupt() and Env_clear_interrupt().

A full regression using this code finished successfully after 16 hours https://buildkite.com/stanford-aha/garnet/builds/5354.

A guide to the file changes

Ten files were changed

     1  .buildkite/ci.yml
     2  tests/test_app/tb/CGRA.cpp
     3  tests/test_app/tb/axil_driver.sv
     4  tests/test_app/tb/axil_ifc.sv
     5  tests/test_app/tb/environment.sv
     6  tests/test_app/tb/garnet_test.sv
     7  tests/test_app/tb/proc_driver.sv
     8  tests/test_app/tb/proc_ifc.sv
     9  tests/test_app/tb/tb_cgra.f
    10  tests/test_app/tb/top.sv

Below: How was each file changed?

ci.yml

Inconsequential changes that do not alter previous functionality.

CGRA.cpp

This is the driver file for Verilator, it invokes the same top.sv module as that used by Vcs.

top.sv

Same as before, except added a block of code to support Verilator wave-tracing to a vcd file.

garnet_test.sv

Rewritten to use global tasks instead of classes.

environment.sv

• Now uses global tasks instead of classes.
• Changed to use non-clocking interface signals e.g. p_ifc.clk instead of vifc_proc.cbd.
• Simplified wait_interrupt() and clear_interrupt() tasks.
• Added conditional one-cycle delays here and there so that VCS and Verilator execution match more closely.

axil_driver.sv, proc_driver.sv

• Global tasks/signals instead of classes.
• Uses non-clocking interface signals only.
• Incorrect non-blocking assignments were rewritten as blocking assignments.
• Better support for debugging (SLAVE_WAIT mechanism etc.).
• A data transfer routine in proc_driver had to be conditioned such that verilator has a one-cycle delay after each word is transferred, while vcs only works correctly if the delay happens before each transfer. I left a "FIXME" comment, somebody should really track this down sometime.

axil_ifc.sv, proc_ifc.sv

These two files had to be rewritten because Verilator did not understnad the "clocking" structure that was being used. Interestingly, changing the code to use normal non-clocking signals seems to work just as well for Vcs as well(!)

tb_cgra.f

This dependency-enumerating support file was rewritten to exclude proc_driver.sv and axil_driver.sv, because these two files are now located via #include pre-processor directive inside environment.sv.

[steveri]

Michael's pull request throws a little monkey wrench into this one #1093

I think what I'd like to do is

• wait for Michael's PR 1093 to go through
• rewrite my code to accommodate the new changes (shouldn't be too hard I think, it only affects top.sv and environment.sv a little bit maybe)
• rerun the full regressions just for paranoia's sake
• finalize this pull (1094) when/if all goes well up to this point

[steveri]

Update

I incorporated changes from Michael's recent PR 1093, the main new things are

• in environment.sv, changed parameter name NUM_CGRA_COLS => NUM_CGRA_COLS_INCLUDING_IO throughout;
• changed Makefile and top.sv to support the new Genesis2-based system.

I ran a full regression on the latest changes, results are here (spoiler alert: they passed):
https://buildkite.com/stanford-aha/garnet/builds/5371

I restarted the failed hetero-cgra-based aha-flow build 10889 using these changes, it is running now as
https://buildkite.com/stanford-aha/aha-flow/builds/10896; for more background on this, see the discussion at
StanfordAHA/aha#1962.

@mcoduoza is the normal CI (plus the additional tests above) good enough to say that this works with your PR-1093 changes, or do we need to do some separate check on that?

[mcoduoza]

Hi Steve, thanks for making these changes. Yes, the normal garnet CI and aha CI are enough to say that this works with my PR.

Could you repoint garnet in your hetero-cgra-fixhttps://github.com/StanfordAHA/aha/tree/hetero-cgra-fix branch to the latest garnet commit here? (i.e. your "slight mod to vcs-test driver" commit).

Once this PR passes the garnet CI and the hetero-cgra-fix passes the aha CI, we are good to go and can merge everything at once.

[steveri]

Could you repoint garnet in your hetero-cgra-fix https://github.com/StanfordAHA/aha/tree/hetero-cgra-fix branch to the latest garnet commit here? (i.e. your "slight mod to vcs-test driver" commit).

Sure no problem. I pushed the changes and the new build is scheduled to run here:
https://buildkite.com/stanford-aha/aha-flow/builds/10897

There is no substantive difference between the 'vcs-test driver' garnet commit and the one that is currently running in build 10896, but it's definitely a good idea to get a clean run with the latest changes. And then I suppose you will want us to file a pull request on hetero-cgra-fix? And/or you can incorporate the changes back to your original hetero-cgra branch and reopen the original PR, whatever you would like to do is fine with me.

[mcoduoza]

And then I suppose you will want us to file a pull request on hetero-cgra-fix? And/or you can incorporate the changes back to your original hetero-cgra branch and reopen the original PR, whatever you would like to do is fine with me.

Yep, we can file a PR directly from hetero-cgra-fix

[steveri]

• All the checks have passed for this PR.
• I still want to wait for https://buildkite.com/stanford-aha/aha-flow/builds/10897 to complete successfully, which should happen sometime before 3pm today I hope.
• Then, when/if I get "at least 1 approving review" I will finalize this merge and open a PR on hetero-cgra-fix.
• Does that sound like a plan?

[mcoduoza]

• Does that sound like a plan?

Sounds good

[mcoduoza]

LGTM