vcs: add support for --workload-list option #293

xiaokamikami · 2024-02-23T06:18:14Z

When you use --workload-list to set up a list of checkpoints or workload, you run through all the workloadin the list in a single launch; Using this feature requires need add WORKLOAD_SWITCH=1 at compile time and enable DPIC functionality
The format of the list should be，Use Spaces as separators
work-path program-run-limit

poemonsense

This function seems not related to checkpoints. For all workloads, the list should work well. If so, then we should name it like workload-list instead of ckpt-list.
Probably we need a CI for this test? The list could be, like, multiple microbench.bin lines.

Let @klin02 have a review and help the CI things

poemonsense · 2024-02-23T06:21:48Z

src/test/csrc/vcs/vcs_main.cpp

+}
+
+static int checkpoint_reset (char * file_name) {
+	int line_len = strlen(file_name);


please format the code like the others

poemonsense · 2024-02-23T06:22:09Z

src/test/csrc/vcs/vcs_main.cpp

+  else 
+    return 1;
+
+  if (sscanf(str1,"_%d_0.%ld_.gz", &checkpoin_idx, &max_instrs) != 2) {


please format the code carefully. E.g., whitespace after ,

poemonsense · 2024-02-23T06:23:47Z

src/test/csrc/vcs/vcs_main.cpp

@@ -58,6 +64,12 @@ extern "C" void set_diff_ref_so(char *s) {
  difftest_ref_so = buf;
 }

+extern "C" void difftest_checkpoint_list (char * path) {
+  checkpoint_list_path = (char *)malloc(256);
+  strcpy(checkpoint_list_path,path);


whitespace after ","

poemonsense · 2024-02-23T06:24:07Z

src/test/csrc/vcs/vcs_main.cpp

@@ -58,6 +64,12 @@ extern "C" void set_diff_ref_so(char *s) {
  difftest_ref_so = buf;
 }

+extern "C" void difftest_checkpoint_list (char * path) {


avoid whitespace between function name and arguments

Maybe u could try format tools like black.

We've adopted clang-format in the master branch. Thanks

xiaokamikami · 2024-02-23T06:28:55Z

This function seems not related to checkpoints. For all workloads, the list should work well. If so, then we should name it like workload-list instead of ckpt-list.

Probably we need a CI for this test? The list could be, like, multiple microbench.bin lines.

Let @klin02 have a review and help the CI things

It's theoretically possible to support different workload lists, but for now I'm only supporting our new checkpoint resolution

poemonsense · 2024-02-23T06:28:56Z

Also, we should create a strict C++ formatter to avoid PRs that bring in inconsistent C++ code style into the repo. @klin02 If you have time, please help find some C++ formatter that is suitable for us. I know Verilator is using the clang formatter for C++

poemonsense · 2024-02-23T06:29:27Z

This function seems not related to checkpoints. For all workloads, the list should work well. If so, then we should name it like workload-list instead of ckpt-list.

Probably we need a CI for this test? The list could be, like, multiple microbench.bin lines.

Let @klin02 have a review and help the CI things

It's theoretically possible to support different workload lists, but for now I'm only supporting our new checkpoint resolution

What are the differences between checkpoints and other workloads? They are simply different images.

poemonsense · 2024-02-23T06:31:06Z

src/test/vsrc/vcs/top.v

@@ -197,8 +224,9 @@ reg [63:0] n_cycles;
 always @(posedge clock) begin
  if (reset) begin
    n_cycles <= 64'h0;
+    xs_rst_en<= 1'b0;


Avoid namings like this (xs). Difftest is not for XS only.

xiaokamikami · 2024-02-23T06:31:43Z

This function seems not related to checkpoints. For all workloads, the list should work well. If so, then we should name it like workload-list instead of ckpt-list.

Probably we need a CI for this test? The list could be, like, multiple microbench.bin lines.

Let @klin02 have a review and help the CI things

It's theoretically possible to support different workload lists, but for now I'm only supporting our new checkpoint resolution

What are the differences between checkpoints and other workloads? They are simply different images.
Because the current limit for the next workload is based on the name in the checkpoint, if I need to run a regular list of applications, I can add a separate macro definition to distinguish between the two modes

poemonsense · 2024-02-23T06:35:11Z

This function seems not related to checkpoints. For all workloads, the list should work well. If so, then we should name it like workload-list instead of ckpt-list.

Probably we need a CI for this test? The list could be, like, multiple microbench.bin lines.

Let @klin02 have a review and help the CI things

It's theoretically possible to support different workload lists, but for now I'm only supporting our new checkpoint resolution

What are the differences between checkpoints and other workloads? They are simply different images.
Because the current limit for the next workload is based on the name in the checkpoint, if I need to run a regular list of applications, I can add a separate macro definition to distinguish between the two modes

I see. Currently this PR is using sscanf(str1,"_%d_0.%ld_.gz", &checkpoin_idx, &max_instrs) to extract the max-instrs.

I think we should avoid this. Probably we can use like workload_path,max_instrs in the workload list to indicate the max-instrs.
Checkpoints are always used as normal memory images in difftest so we should not use the filename to extract the information.

klin02 · 2024-02-23T10:51:40Z

Also, we should create a strict C++ formatter to avoid PRs that bring in inconsistent C++ code style into the repo. @klin02 If you have time, please help find some C++ formatter that is suitable for us. I know Verilator is using the clang formatter for C++

Sure. I will try to handle it.

poemonsense · 2024-02-23T12:31:01Z

.github/workflows/main.yml

+
+      - name: Verilator Build with VCS Top (with workload list)
+        run : |
+            echo "TEST_HOME=/nfs/home/share/ci-workloads/workload-list-test" >> $GITHUB_ENV


Do not use internal server files in CI. CI runs on GitHub servers. We need to setup a workload list in CI and then use it.

For example:

echo "./ready-to-run/microbench.bin,10000" > list.txt echo "./ready-to-run/linux.bin,20000" >> list.txt ./build/simv +workload-list=list.txt +diff=./ready-to-run/riscv64-nemu-interpreter-so

ok,i get it

poemonsense · 2024-02-23T12:32:15Z

.github/workflows/main.yml

+            source ./env.sh
+            make clean
+            make simv DIFFTEST_PERFCNT=1 VCS=verilator -j2
+            ./build/simv  +workload=./ready-to-run/microbench.bin +e=0 +diff=./ready-to-run/riscv64-nemu-interpreter-so +max-instrs=10000 +workload-list=$TEST_HOME/list.txt


I think when +workload-list is given, we don't need +workload any more. Similarly, with workload-list specifying the max-instrs, we do not need to set +max-instrs again. Is it?

Also please add \n to the end of file. In the Files changed tab, GitHub shows a red mark here which means the code is missing the \n at the end of the file.

In VSCode, it is added by adding a new blank line at the end of the file. In vim, it should work well without explicit \n.

poemonsense

I think the PR is clear. However, some details should be improved and then we can merge it.

klin02 · 2024-02-23T14:11:29Z

src/test/csrc/vcs/vcs_main.cpp

@@ -58,6 +63,12 @@ extern "C" void set_diff_ref_so(char *s) {
  difftest_ref_so = buf;
 }

+extern "C" void difftest_workload_list(char * path) {


Use similar function name like set_workload_list() for arg passing. difftest_xxx is more like a func name in difftest.cpp.

klin02 · 2024-02-23T14:18:19Z

src/test/csrc/vcs/vcs_main.cpp

 static bool has_reset = false;
 static char bin_file[256] = "ram.bin";
 static char *flash_bin_file = NULL;
 static bool enable_difftest = true;
 static uint64_t max_instrs = 0;

+static int ram_path_reset(char *file_name);


Seems no need to declare a func as static ?

Only this file will use this function, so can use static

klin02 · 2024-02-23T14:48:38Z

I have some questions about Memory reload and Software reload.
For memory:

Currently, we use $fread to load bin into synthesizable memory in initial block. When reset is set high again, this will not be acted again to reload DUT memory. See details in https://github.com/OpenXiangShan/difftest/blob/b4d9e66fe345007ca401ea9edeb1f1c98db81524/src/main/scala/common/Mem.scala#L41C14-L41C26.
Now we use synthesizable memory instead of DPIC by default. It's because DPIC with input and output will greatly slow down simulation at every calls. If you want to reload memory, maybe you can try extern DPIC function. Please think it carefully for clear structure.

For software:
It seems each simulation circuits can be seen like: begins with reset set to high (you will trigger simv_init in cycle0),ends with a non-zero simv_nstep result.

Because simv_init will trigger several times, you should make sure previous resources is freed succesfully. Every circuits should be seperated with each other.
I think logic in top.v is mixed. Only a singal (seems like soft_rst_en) should be added to control reset. When a ckpt is ended, this signal will rise reset again. It will not affect any other logic.

Overall, I think the code structure will be like this:

When initial, you pass workload-list, and parse it to get workload array and max-instr array.
When reset is set to high. You will trigger simv_init, it will get workload and max-instr, just like only a workload is run. You will init software in this. And you may use extern DPIC function to load bin into DUT synthesizable memory.
When simv_nstep is ended successfully, and there is still remain ckpt. It will use a signal to set reset to high. And Software should be free clearly. Then Back to Point 2.
When last simv_nstep is ended (maybe return val is different from Point 3). The whole simulation is ended.

poemonsense · 2024-02-23T14:48:39Z

It seems CI test-difftest-vcs fails but does not abort:

diff-test ref so:./ready-to-run/riscv64-nemu-interpreter-so
set checkpoint list path list.txt 
Can't open fp file 'list.txt'run work list start faild
- /home/runner/work/difftest/xs-env/NutShell/difftest/src/test/vsrc/vcs/top.v:138: Verilog $finish

Did you try the CI tests on local machine? You can test it with local servers using Verilator.

klin02 · 2024-02-23T15:00:17Z

src/test/vsrc/vcs/top.v

@@ -215,7 +247,12 @@ always @(posedge clock) begin
 `ifdef CONFIG_DIFFTEST_DEFERRED_RESULT
    else if (simv_result) begin
      $display("DIFFTEST FAILED at cycle %d", n_cycles);
-      $finish();
+      if (simv_result == STATE_LIMIT_EXCEEDED && workload_list_en == 1'b1)begin


simv_result is only used when define CONFIG_DIFFTEST_DEFERRED_RESULT. If it is not defined, it seems ended at first workload.

poemonsense · 2024-02-28T01:05:43Z

.github/workflows/main.yml

+            echo "./ready-to-run/linux.bin 20000" >> list.txt
+            make simv VCS=verilator WORKLOAD_SWITCH=1 -j2
+            ./build/simv +e=0 +diff=./ready-to-run/riscv64-nemu-interpreter-so +workload-list=./list.txt
+            rm -rf ./ready-to-run/list.txt


No need to remove.

* CI: add clang-format for github actions We add clang-format to ensure consistent Cpp code style. When some code is no need to formatter, use clang-format off/on to control it. Now our clang-format config is based on LLVM style, which can be checked by clang-format -style=LLVM --dump-config. All possible options and their meanings can be searched in https://clang.llvm.org/docs/ClangFormatStyleOptions.html After this PR, github actions will check format automatically. When the code need format, CI will fail, user need to make format manully and commit again.

klin02 · 2024-03-01T03:09:47Z

src/test/vsrc/vcs/DeferredControl.v

@@ -31,7 +31,10 @@ initial $ixc_ctrl("sfifo", "set_deferred_result");
 `endif // PALLADIUM

 always @(posedge clock) begin
-  if (!reset && step != 0) begin
+  if (reset) begin
+    simv_result = 1'b0;


Use <= instead of =

klin02 · 2024-03-01T03:10:19Z

src/test/vsrc/vcs/top.v

+  // set exit instrs const
+  if ($test$plusargs("max-instrs")) begin
+    $value$plusargs("max-instrs=%d", max_instrs);
+    set_max_instrs(max_instrs);


I had changed this in another PR #298

klin02 · 2024-03-01T03:14:23Z

src/test/csrc/vcs/vcs_main.cpp

@@ -117,6 +117,7 @@ extern "C" uint8_t simv_init() {
    init_goldenmem();
    init_nemuproxy(DEFAULT_EMU_RAM_SIZE);
  }
+  simv_result = SIMV_RUN;


No need to change this? The simv_result is get from every simv_nstep. We declared it outside just becuase when some workload failed. No more simv_step should act.

vcs: run checkpoint list

bd9168b

xiaokamikami requested a review from poemonsense February 23, 2024 06:20

poemonsense reviewed Feb 23, 2024

View reviewed changes

poemonsense requested a review from klin02 February 23, 2024 06:27

poemonsense reviewed Feb 23, 2024

View reviewed changes

xiaokamikami changed the title ~~vcs: run checkpoint list for --ckpt-list~~ vcs: run checkpoint list for --workload-list Feb 23, 2024

xiaokamikami changed the title ~~vcs: run checkpoint list for --workload-list~~ vcs: run workload list for --workload-list Feb 23, 2024

xiaokamikami force-pushed the master branch 5 times, most recently from 58bf07c to 8f8667f Compare February 23, 2024 10:04

xiaokamikami added 2 commits February 23, 2024 18:04

vcs: add the versatility of soft reset

8f8667f

ci: add workload-list ci

5d1d344

poemonsense reviewed Feb 23, 2024

View reviewed changes

klin02 reviewed Feb 23, 2024

View reviewed changes

xiaokamikami added 2 commits February 23, 2024 22:34

vcs: Initialize ram with workload-list

2fc2e7f

CI: worklist ci cancels using workload to run

6edd3a2

klin02 reviewed Feb 23, 2024

View reviewed changes

poemonsense reviewed Feb 28, 2024

View reviewed changes

CI: remove redundancy

62bc714

xiaokamikami dismissed klin02’s stale review via 62bc714 February 28, 2024 02:01

klin02 previously approved these changes Feb 28, 2024

View reviewed changes

xiaokamikami dismissed klin02’s stale review via 93b627a February 28, 2024 03:22

xiaokamikami force-pushed the master branch 2 times, most recently from a47dfc6 to 198e602 Compare February 28, 2024 03:31

xiaokamikami and others added 4 commits February 28, 2024 11:32

vcs: add dpic memory define

198e602

vcs: fix on simv_result assign

d0ca521

vcs: fix workload_switch on wire faild and move max instrs

e0ea726

xiaokamikami force-pushed the master branch 3 times, most recently from 46a9a3e to 1253d95 Compare March 1, 2024 03:06

klin02 reviewed Mar 1, 2024

View reviewed changes

xiaokamikami force-pushed the master branch 6 times, most recently from 9487f9a to d4dcf7b Compare March 1, 2024 04:38

xiaokamikami added 2 commits March 1, 2024 12:39

vcs: modify format

d4dcf7b

Merge branch 'master' into master

5ca5029

klin02 requested a review from poemonsense March 1, 2024 05:39

poemonsense approved these changes Mar 1, 2024

View reviewed changes

klin02 changed the title ~~vcs: run workload list for --workload-list~~ vcs: add support for --workload_list option Mar 1, 2024

klin02 changed the title ~~vcs: add support for --workload_list option~~ vcs: add support for --workload-list option Mar 1, 2024

klin02 merged commit 1a0a0a0 into OpenXiangShan:master Mar 1, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vcs: add support for --workload-list option #293

vcs: add support for --workload-list option #293

xiaokamikami commented Feb 23, 2024 •

edited

Loading

poemonsense left a comment

poemonsense Feb 23, 2024

poemonsense Feb 23, 2024

poemonsense Feb 23, 2024

poemonsense Feb 23, 2024

linkedlist771 Mar 1, 2024

poemonsense Mar 1, 2024

xiaokamikami commented Feb 23, 2024

poemonsense commented Feb 23, 2024

poemonsense commented Feb 23, 2024

poemonsense Feb 23, 2024

xiaokamikami commented Feb 23, 2024

poemonsense commented Feb 23, 2024 •

edited

Loading

klin02 commented Feb 23, 2024

poemonsense Feb 23, 2024

xiaokamikami Feb 23, 2024

poemonsense Feb 23, 2024

poemonsense Feb 23, 2024

poemonsense left a comment

klin02 Feb 23, 2024

klin02 Feb 23, 2024

xiaokamikami Feb 23, 2024

klin02 commented Feb 23, 2024

poemonsense commented Feb 23, 2024

klin02 Feb 23, 2024

poemonsense Feb 28, 2024

klin02 Mar 1, 2024

klin02 Mar 1, 2024

klin02 Mar 1, 2024

vcs: add support for --workload-list option #293

vcs: add support for --workload-list option #293

Conversation

xiaokamikami commented Feb 23, 2024 • edited Loading

poemonsense left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaokamikami commented Feb 23, 2024

poemonsense commented Feb 23, 2024

poemonsense commented Feb 23, 2024

Choose a reason for hiding this comment

xiaokamikami commented Feb 23, 2024

poemonsense commented Feb 23, 2024 • edited Loading

klin02 commented Feb 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poemonsense left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klin02 commented Feb 23, 2024

poemonsense commented Feb 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaokamikami commented Feb 23, 2024 •

edited

Loading

poemonsense commented Feb 23, 2024 •

edited

Loading