zachjs / sv2v

SystemVerilog to Verilog conversion
BSD 3-Clause "New" or "Revised" License
540 stars 52 forks source link

Test correct and incorrect type parameters from CVA6 #281

Open jrrk2 opened 5 months ago

jrrk2 commented 5 months ago

This patch contains a standalone set of Ariane (CVA6) System Verilog RISCV processor files to test the type parameter bug in sv2v.

sv2v/test/type_param/sv2v.sh contains the unpatched files and fails with this error:

sv2v: field 'insn' not found in struct packed { logic x_compressed_valid; struct packed { logic [15:0] instr; logic [1:0] mode; logic [2:0] id; } x_compressed_req; logic x_issue_valid; struct packed { logic [31:0] instr; logic [1:0] mode; logic [2:0] id; logic [1:0][63:0] rs; logic [1:0] rs_valid; } x_issue_req; logic x_commit_valid; struct packed { logic [2:0] id; logic x_commit_kill; } x_commit; logic x_mem_ready; struct packed { logic exc; logic [5:0] exccode; } x_mem_resp; logic x_mem_result_valid; struct packed { logic [2:0] id; logic [63:0] rdata; logic err; } x_mem_result; logic x_result_ready; }, in expression acc_req_o.insn, within scope acc_dispatcher_20306, near core/acc_dispatcher.sv:213:3 CallStack (from HasCallStack): error, called at src/Convert/Scoper.hs:376:22 in main:Convert.Scoper

possibly because the converter tries to find struct fields in a type parameter prior to substitution.

The alternative script:

sv2v_patched/test/type_param/sv2v_corrected.sh

uses a patched core/acc_dispatcher.sv which substitutes the default type parameter value directly:

70,71c70,71
<     output acc_req_t acc_req_o,
<     input acc_resp_t acc_resp_i
---
>     output acc_pkg::accelerator_req_t acc_req_o,
>     input acc_pkg::accelerator_resp_t acc_resp_i

and this generates a plausible output file:

-rw-r--r-- 1 jonathan staff 7496480 23 Apr 10:50 cva6_nonsys.v

However, I don't have a suitable test bench or other methodology to add to the regression suite yet.

zachjs commented 4 months ago

Thanks for sharing this test case! After minimizing the input, I believe this is the sane as #265.

jrrk2 commented 4 months ago

That may be so, and the fix could be complicated, However if you elaborate all the type parameters before searching for record fields then I think the problem would go away.

zachjs commented 4 months ago

I've attached my "minimized" version of your example. out.sv.txt

To me, it looks like .insn will be accessed so long as sv2v cannot determine that CVA6ExtendCfg.EnableAccelerator is false. At elaboration time, sv2v elaborates type parameters, not regular parameters, so it can't trivially make this assumption. However, if assign acc_req_o.insn = acc_req_int.insn; were replaced with $error("... not in struct");, I think this could be read into Yosys just fine. What am I missing? I'm imagining a mode where sv2v replaces a "missing struct field" with an elaboration system task, rather than immediately raising a conversion error.