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

Test correct and incorrect type parameters from CVA6 #281

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jrrk2
Copy link

@jrrk2 jrrk2 commented Apr 23, 2024

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 and others added 6 commits April 14, 2024 16:19
I'm opting for iverilog's interpretation of the specifications here. The
commercial simulators I tested seem to agree.
The latest verion of iverilog enforces declaration ordering more
strictly. Update a few test cases to match. sv2v still supports
out-of-order items on a best-effort basis.
- refresh GitHub Actions versions
- manually install shUnit2 v2.1.8
- more portable usage check
- run-all.sh exits on ctrl-c
@zachjs
Copy link
Owner

zachjs commented May 14, 2024

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

@jrrk2
Copy link
Author

jrrk2 commented May 14, 2024

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
Copy link
Owner

zachjs commented May 14, 2024

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.

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.

3 participants