Conversation
Hotaru71
commented
Apr 3, 2026
- Add sv_extern_mode option to config for verification routing
- Update compileDriver with verification_run and make_config
- Update main.ml to route to verification path when -sv-extern is passed
- Add assertName module for verification naming
- Update codegen and codegenPort with verification codegen functions
- Add one example called extern_example1_sender.anvil
|
@Hotaru71, please list down the properties for which assertions are added. Thanks. |
There was a problem hiding this comment.
Dont add the generated sv files to commit
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “verification mode” compilation path (triggered via -sv-extern) that generates verification-oriented SystemVerilog (assertion wrappers), along with supporting naming/helpers and an example.
Changes:
- Added
sv_extern_modeto the compiler config / CLI parsing and routedmaininto a new verification driver path. - Added
CompileDriver.verification_run+ verification codegen entrypoints (Codegen.verification_generate*), plus port helper utilities for verification. - Added an
AssertNamehelper module and a new extern sender example.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/config.mli | Extends compile_config with sv_extern_mode. |
| lib/config.ml | Parses -sv-extern and stores sv_extern_mode in config. |
| bin/main.ml | Routes to verification mode when sv_extern_mode is set. |
| lib/compileDriver.mli | Exposes verification_run API. |
| lib/compileDriver.ml | Adds verification compilation pipeline and config builder. |
| lib/codegenPort.mli | Adds verification port formatting/name utilities. |
| lib/codegenPort.ml | Implements verification port formatting/name utilities. |
| lib/codegen.mli | Exposes verification codegen entrypoints. |
| lib/codegen.ml | Implements verification SV generation (assertion wrappers, includes, instantiations). |
| lib/assertName.mli | New module API for verification naming. |
| lib/assertName.ml | New module implementation for verification naming. |
| examples/extern_example1_sender.anvil | Adds a new extern/verification-related example input. |
| examples/extern_example1_sender.anvil.sv | Adds the corresponding generated SV example artifact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match msg_def.sig_types with | ||
| | [] -> Printf.printf "parameter int N = #;\n" | ||
| | stype0 :: _ -> | ||
| match number_of_lifetime_cycles stype0.lifetime.e with | ||
| | Some n -> Printf.printf "parameter int N = %d;\n" n | ||
| | None -> Printf.printf "parameter int N = #;\n" | ||
| else () | ||
| in |
There was a problem hiding this comment.
The lifetime-bound emission uses Printf.printf (stdout) and prints placeholder text like "parameter int N = #;", which won’t end up in the generated verification SystemVerilog and also produces invalid SV when lifetime cycles aren’t Cycles n. This should be emitted via CodegenPrinter/Out_channel (and avoid generating invalid syntax; e.g., skip the parameter or raise a compile error when the bound can’t be determined).
| match msg_def.sig_types with | |
| | [] -> Printf.printf "parameter int N = #;\n" | |
| | stype0 :: _ -> | |
| match number_of_lifetime_cycles stype0.lifetime.e with | |
| | Some n -> Printf.printf "parameter int N = %d;\n" n | |
| | None -> Printf.printf "parameter int N = #;\n" | |
| else () | |
| in | |
| match msg_def.sig_types with | |
| | [] -> | |
| failwith | |
| (Printf.sprintf | |
| "Cannot emit lifetime bound parameter for endpoint %s: message has no signal types." | |
| ep.name) | |
| | stype0 :: _ -> | |
| match number_of_lifetime_cycles stype0.lifetime.e with | |
| | Some n -> | |
| CodegenPrinter.print_line printer | |
| (Printf.sprintf "parameter int N = %d;" n) | |
| | None -> | |
| failwith | |
| (Printf.sprintf | |
| "Cannot emit lifetime bound parameter for endpoint %s: lifetime bound is not statically determinable." | |
| ep.name) | |
| else () | |
| in |
| | (_, Dynamic) -> "typedef enum logic [1:0] {WAIT_ACK, DROP_ACK} state_t;" | ||
| | (Dynamic, _) -> "typedef enum logic [1:0] {WAIT_REQ, DROP_VALID} state_t;" | ||
| | _ -> "None" |
There was a problem hiding this comment.
Returning the literal string "None" for unsupported sync combinations will be printed into the generated SystemVerilog (via print_declaration_FSM), which will break compilation. Consider returning an empty string and skipping printing, or raising an error for unsupported combinations instead of emitting "None".
| ( | ||
| match g.extern_module, g.proc_body with | ||
| | _ -> | ||
| let initEvents = fst @@ List.hd g.threads in | ||
| codegen_endpoints printer graphs initEvents; | ||
| ); | ||
|
|
||
| (* Retreive the user module name *) | ||
| let s = AssertName.user_sv () in | ||
| (* Print user module instantiation *) | ||
| Printf.sprintf "%s user_sv (" s |> CodegenPrinter.print_line printer; | ||
| let _ = verification_codegen_instantiations printer graphs g.messages.args in | ||
| ( | ||
| match g.extern_module, g.proc_body with | ||
| | _ -> | ||
| let initEvents = fst @@ List.hd g.threads in | ||
| codegen_endpoints printer graphs initEvents; | ||
| ); |
There was a problem hiding this comment.
codegen_endpoints is called after verification_codegen_ports already emitted declarations, and it’s invoked multiple times (before user instantiation, before anvil instantiation, etc.). Since codegen_endpoints prints signal declarations, this will cause duplicate declarations in the generated SV. Consider removing these calls or refactoring to emit endpoint declarations exactly once (and emit only connections inside instantiations).
| ( | |
| match g.extern_module, g.proc_body with | |
| | _ -> | |
| let initEvents = fst @@ List.hd g.threads in | |
| codegen_endpoints printer graphs initEvents; | |
| ); | |
| (* Retreive the user module name *) | |
| let s = AssertName.user_sv () in | |
| (* Print user module instantiation *) | |
| Printf.sprintf "%s user_sv (" s |> CodegenPrinter.print_line printer; | |
| let _ = verification_codegen_instantiations printer graphs g.messages.args in | |
| ( | |
| match g.extern_module, g.proc_body with | |
| | _ -> | |
| let initEvents = fst @@ List.hd g.threads in | |
| codegen_endpoints printer graphs initEvents; | |
| ); | |
| (* Retreive the user module name *) | |
| let s = AssertName.user_sv () in | |
| (* Print user module instantiation *) | |
| Printf.sprintf "%s user_sv (" s |> CodegenPrinter.print_line printer; | |
| let _ = verification_codegen_instantiations printer graphs g.messages.args in |
| String.concat "\n" [ | ||
| Printf.sprintf "`include \"nosyn_user_sender.svh\"\n"; | ||
| Printf.sprintf "assert property (data_stable_valid_high(%s, state_curr, %s))" valid_name data; |
There was a problem hiding this comment.
The generated verification SV includes `include directives for .svh files (e.g. nosyn_user_sender.svh) that don’t exist anywhere in this repository. Unless these files are generated/installed elsewhere, the verification output will not compile; consider adding them to the repo or documenting/embedding these properties so the output is self-contained.
| Printf.sprintf "assert property (valid_low_during_DROP_VALID (state_curr, %s)" valid_name; | ||
| Printf.sprintf "else $error(\"Assertion Failed: data_stable_N_cycles\");\n"; |
There was a problem hiding this comment.
In the (Dynamic, _) / Left case, the generated assertion string is syntactically incomplete (missing closing parenthesis / terminator on the valid_low_during_DROP_VALID property), which will produce invalid SystemVerilog. Also the subsequent $error message references the wrong assertion name. Please fix the emitted assertion line so it’s valid SV and the error label matches the property.
| Printf.sprintf "assert property (valid_low_during_DROP_VALID (state_curr, %s)" valid_name; | |
| Printf.sprintf "else $error(\"Assertion Failed: data_stable_N_cycles\");\n"; | |
| Printf.sprintf "assert property (valid_low_during_DROP_VALID (state_curr, %s))" valid_name; | |
| Printf.sprintf "else $error(\"Assertion Failed: valid_low_during_DROP_VALID\");\n"; |
| let verification_compile out config = | ||
| let open Config in | ||
| let toplevel_filename = List.hd config.input_filenames in | ||
| let cunits = ref [] in |
There was a problem hiding this comment.
verification_compile doesn’t mirror the normal compile path’s behavior: it ignores config.just_check and does not emit the standard SV preamble (Codegen.generate_preamble). If verification output is meant to be compiled/simulated similarly, consider reusing the same preamble + just_check gating so behavior is consistent across modes.
| | anvil_file :: gen_sv_file :: user_sv_file :: _ -> | ||
| AssertName.generate gen_sv_file user_sv_file; | ||
| let c = make_config anvil_file in | ||
| (try verification_compile stdout c | ||
| with |
There was a problem hiding this comment.
verification_run rebuilds a fresh config via make_config, which drops user-specified flags from the original CLI config (e.g., -verbose, -disable-lt-checks, -O, -two-round, etc.) and also ignores sv_extern_mode’s value entirely. Consider deriving the verification config from the passed-in config (overriding only input_filenames/output handling) so verification behaves consistently with normal compilation and the mode value is actually used.
| ("-json", Arg.Set json_output, "Output compilation results in JSON format"); | ||
| ("-sv-extern", Arg.Set_string sv_extern_mode, "SV extern mode: extern (default), anvil, all, none") | ||
| ] | ||
| add_input_filename | ||
| "anvil [-stdin] [-verbose] [-disable-lt-checks] [-O <opt-level>] [-two-round] [-json] [-strict-dtc] <file1> [<file2>] ..."; |
There was a problem hiding this comment.
The -sv-extern flag is parsed as a required string argument and documented as having a default of "extern", but the actual default is the empty string and the value is only used as a boolean trigger. Also the Arg.parse usage string printed on -help does not mention -sv-extern. Consider either (a) making -sv-extern a boolean flag if it only enables verification routing, or (b) validating/using the mode string (Arg.Symbol / custom parser) and updating the help/usage text accordingly.
| let user_sv () : string = | ||
| match !user_sv_ref with | ||
| | Some s -> strip_sv_name s | ||
| | None -> failwith "AssertName.sender: not initialised (call AssertName.generate first)" |
There was a problem hiding this comment.
The failure message references AssertName.sender even though the function is user_sv; this makes debugging initialization failures confusing. Consider updating the message to match the function/module names and be consistent with the other error strings.
| | None -> failwith "AssertName.sender: not initialised (call AssertName.generate first)" | |
| | None -> failwith "AssertName.user_sv: not initialised (call AssertName.generate first)" |
| | Some s -> s | ||
| | None -> failwith "AssertName.receiver: not initialised (call AssertName.generate first)" |
There was a problem hiding this comment.
anvil_sv currently returns the stored string unchanged (likely a file path), unlike user_sv which strips to a basename without extension. If anvil_sv is intended to be used as a SystemVerilog module identifier, it should probably apply strip_sv_name as well. Also the failure message references AssertName.receiver, which doesn’t match the function name.
| | Some s -> s | |
| | None -> failwith "AssertName.receiver: not initialised (call AssertName.generate first)" | |
| | Some s -> strip_sv_name s | |
| | None -> failwith "AssertName.anvil_sv: not initialised (call AssertName.generate first)" |
|
@Hotaru71 seems like this is a WIP. Copilot flagged some functional correctness issues ? You can take a look and fix them if they are correct. Once done, I can review in detail. |
The following assertion properties have been added:
|
There was a problem hiding this comment.
u can seperate the added code for wrapper generation in different module
|
|
||
| (** Generate code for a {!type:EventGraph.event_graph_collection} to a specified output channel. *) | ||
| val generate : out_channel -> Config.compile_config -> EventGraph.event_graph_collection -> unit | ||
|
|
Can u please add these in a markdown file as a readme in properties (lib) directory (where u have assertions) Also add more test cases in that case ? I see only one |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match List.rev config.input_filenames with | ||
| | anvil_file :: gen_sv_file :: user_sv_file :: _ -> | ||
| AssertName.generate gen_sv_file user_sv_file; | ||
| let c = { config with | ||
| input_filenames = [anvil_file]; | ||
| output_filename = None; | ||
| } in |
There was a problem hiding this comment.
verification_run requires three input files per the error message, but gen_sv_file is only used to initialise AssertName.anvil_sv_ref (and AssertName.anvil_sv() isn’t referenced anywhere in the verification codegen). As written, the second argument has no functional effect and the tool still prints verification output to stdout, so requiring it is misleading. Either use gen_sv_file as an actual output path (write generated SV there) or drop it from the required inputs / error message.
| function | ||
| | Text msg_text -> Printf.eprintf "%s\n" msg_text | ||
| | Codespan (file_name, span) -> ( | ||
| let file_name = Option.get file_name in | ||
| Printf.eprintf "%s:%d:%d:\n" file_name span.st.pos_lnum (span.st.pos_cnum - span.st.pos_bol); | ||
| ) |
There was a problem hiding this comment.
This error-printing path does Option.get file_name for Codespan fragments, but Codespan explicitly allows filename to be None (e.g. Except.codespan_local). If a compile error includes a Codespan (None, ...), this will raise and mask the real compilation error. Handle the None case (print span without filename, or skip the location line) instead of using Option.get.
| List.concat_map (fun (msg : message_def) -> | ||
| let msg = ParamConcretise.concretise_message cc.params endpoint.channel_params msg in | ||
| List.mapi (fun i (_stype : sig_type_chan_local) -> | ||
| CodegenFormat.format_msg_data_signal_name endpoint.name msg.name i | ||
| ) msg.sig_types | ||
| ) cc.messages |
There was a problem hiding this comment.
data_port_names currently generates a data signal name for every msg.sig_types entry, even when the corresponding signal type is unit. Those unit-typed signals are not emitted as ports elsewhere (e.g. gather_ports_from_endpoint skips unit_dtype), so this can produce names for signals that do not exist and break the generated verification SV. Filter out unit-typed signal types (or reuse message_has_data_port / gather_ports ordering) when building data_port_names.
| (* The list of signals, later use for indexing *) | ||
| let valid_names = CodegenPort.valid_port_names graphs.channel_classes g.messages.args in | ||
| let ack_names = CodegenPort.ack_port_names graphs.channel_classes g.messages.args in | ||
| let data_names = CodegenPort.data_port_names graphs.channel_classes g.messages.args in | ||
|
|
There was a problem hiding this comment.
The valid_names / ack_names lists are built by filtering messages by whether they have a valid/ack port, but later they’re indexed by count which ranges over all messages (total_count). If a channel class mixes sync modes across messages, indexing will misalign (or fall back to ""), generating invalid SV (e.g. if ()) or referencing the wrong signal. Build a per-message mapping (e.g. by iterating messages with their original indices and producing string option slots) so count always maps to the correct valid/ack/data signal names for that specific message.
| property data_stable_N_cycles_after_ack_high (current_state, counter_0, data); | ||
| @(posedge clk_i) ((current_state==DROP_VALID || counter_0!=0) |-> (_endp_res_0 == $past(data))); | ||
| endproperty |
There was a problem hiding this comment.
This property references _endp_res_0, which is not declared anywhere in the property arguments or in the generated wrapper modules, so the included SVH will not compile. It looks like this should reference the data argument (or data_0) consistently instead of _endp_res_0.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CodegenPrinter.print_line printer ".clk_i(clk_i),"; | ||
| CodegenPrinter.print_line printer ".rst_ni(rst_ni),"; | ||
| let rec print_port_list port_list = | ||
| match port_list with | ||
| | [] -> () | ||
| | port :: [] -> | ||
| let s = CodegenPort.instanformat port in | ||
| Printf.sprintf ".%s (%s)" s s |> CodegenPrinter.print_line printer | ||
| | port :: port_list' -> | ||
| let s = CodegenPort.instanformat port in | ||
| Printf.sprintf ".%s (%s)," s s |> CodegenPrinter.print_line printer; | ||
| print_port_list port_list' | ||
| in | ||
| print_port_list port_list; |
There was a problem hiding this comment.
verification_codegen_instantiations always prints .clk_i(...), and .rst_ni(...), with trailing commas. If port_list is empty (e.g. a proc has no non-unit message ports and no dynamic sync ports), the generated instance will end with a dangling comma before );, which is invalid SystemVerilog. Handle the empty list case by omitting the trailing comma(s) or by printing the last connection without a comma.
| CodegenPrinter.print_line printer ".clk_i(clk_i),"; | |
| CodegenPrinter.print_line printer ".rst_ni(rst_ni),"; | |
| let rec print_port_list port_list = | |
| match port_list with | |
| | [] -> () | |
| | port :: [] -> | |
| let s = CodegenPort.instanformat port in | |
| Printf.sprintf ".%s (%s)" s s |> CodegenPrinter.print_line printer | |
| | port :: port_list' -> | |
| let s = CodegenPort.instanformat port in | |
| Printf.sprintf ".%s (%s)," s s |> CodegenPrinter.print_line printer; | |
| print_port_list port_list' | |
| in | |
| print_port_list port_list; | |
| let connections = | |
| [ | |
| ".clk_i(clk_i)"; | |
| ".rst_ni(rst_ni)"; | |
| ] @ List.map (fun port -> | |
| let s = CodegenPort.instanformat port in | |
| Printf.sprintf ".%s (%s)" s s | |
| ) port_list | |
| in | |
| let rec print_connections connections = | |
| match connections with | |
| | [] -> () | |
| | [connection] -> | |
| CodegenPrinter.print_line printer connection | |
| | connection :: connections' -> | |
| CodegenPrinter.print_line printer (connection ^ ","); | |
| print_connections connections' | |
| in | |
| print_connections connections; |
| property data_stable_valid_high (valid, current_state, data); | ||
| @(posedge clk_i) ((valid && !$rose(valid)) && (current_state inside {WAIT_REQ, WAIT_ACK})) |-> data == $past(data); | ||
| endproperty |
There was a problem hiding this comment.
These properties use $past(...) without any reset gating. On the first sampled clock edge (and during/just after reset), $past and state/data signals can be X, which can cause spurious assertion failures in simulators/formal tools. Consider adding disable iff (!rst_ni) to the property clocking event (or otherwise guarding against reset/first-cycle sampling).
| @@ -0,0 +1,3 @@ | |||
| property data_stable_N_cycles (state_curr_0, counter_0, data_0); | |||
| @(posedge clk_i) ((state_curr_0==DROP_ACK || counter_0!=0) |-> (data_0 == $past(data_0))); | |||
There was a problem hiding this comment.
These properties use $past(...) without any reset gating, which can cause X-propagation/spurious failures around reset or the first sampled edge. Consider adding disable iff (!rst_ni) (or an equivalent guard) to the property clocking event.
| @(posedge clk_i) ((state_curr_0==DROP_ACK || counter_0!=0) |-> (data_0 == $past(data_0))); | |
| @(posedge clk_i) disable iff (!rst_ni) ((state_curr_0==DROP_ACK || counter_0!=0) |-> (data_0 == $past(data_0))); |
| ## Examples | ||
|
|
||
| You can try the assertion flow using the provided example designs in the `examples` directory. There are 6 examples, with filenames in the format: | ||
|
|
||
| - `extern_exampleN_sender.anvil` / `extern_exampleN_receiver.sv` | ||
| - or `extern_exampleN_receiver.anvil` / `extern_exampleN_sender.sv` | ||
|
|
There was a problem hiding this comment.
The PR description says it "Add[s] one example", but this change adds multiple extern_example* files (and the README documents 6 examples). Please update the PR description (or the README) so they agree on how many examples are included.
| ("-json", Arg.Set json_output, "Output compilation results in JSON format"); | ||
| ("-sv-extern", Arg.Set_string sv_extern_mode, "SV extern mode: extern") | ||
| ] | ||
| add_input_filename | ||
| "anvil [-stdin] [-verbose] [-disable-lt-checks] [-O <opt-level>] [-two-round] [-json] [-strict-dtc] <file1> [<file2>] ..."; | ||
| "anvil [-stdin] [-verbose] [-disable-lt-checks] [-O <opt-level>] [-two-round] [-json] [-strict-dtc] [-sv-extern] <file1> [<file2>] ..."; |
There was a problem hiding this comment.
The help/usage string lists [-sv-extern] as if it takes no argument, but the flag is defined with Arg.Set_string so it requires a mode argument (e.g. -sv-extern extern). Update the usage string (and ideally the flag help text) to reflect the required <mode> parameter to avoid confusing CLI users.
| @@ -0,0 +1,3 @@ | |||
| property data_stable_N_cycles (state_curr_0, counter_0, data_0); | |||
| @(posedge clk_i) ((state_curr_0==DROP_VALID || counter_0!=0) |-> (data_0 == $past(data_0))); | |||
There was a problem hiding this comment.
These properties use $past(...) without any reset gating. On the first sampled clock edge (and during/just after reset), $past and state/data signals can be X, which can cause spurious assertion failures. Consider adding disable iff (!rst_ni) (or an equivalent guard) to the property clocking event.
| @(posedge clk_i) ((state_curr_0==DROP_VALID || counter_0!=0) |-> (data_0 == $past(data_0))); | |
| @(posedge clk_i) disable iff (!rst_ni) ((state_curr_0==DROP_VALID || counter_0!=0) |-> (data_0 == $past(data_0))); |
| endproperty | ||
|
|
||
| property data_stable_N_cycles (state_curr_0, counter_0, data_0); | ||
| @(posedge clk_i) ((state_curr_0==DROP_ACK || counter_0!=0) |-> (data_0 == $past(data_0))); |
There was a problem hiding this comment.
These properties use $past(...) without any reset gating, which can cause X-propagation/spurious failures around reset or the first sampled edge. Consider adding disable iff (!rst_ni) (or an equivalent guard) to the property clocking event.
| @(posedge clk_i) ((state_curr_0==DROP_ACK || counter_0!=0) |-> (data_0 == $past(data_0))); | |
| @(posedge clk_i) disable iff (!rst_ni) ((state_curr_0==DROP_ACK || counter_0!=0) |-> (data_0 == $past(data_0))); |
| @(posedge clk_i) ((state_curr_0==DROP_VALID || counter_0!=0) |-> (data_0 == $past(data_0))); | ||
| endproperty | ||
|
|
||
| property valid_low_during_DROP_VALID (state_curr_0, valid_0); | ||
| @(posedge clk_i) (state_curr_0==DROP_VALID) |-> !valid_0; |
There was a problem hiding this comment.
These properties use $past(...) without any reset gating, which can cause X-propagation/spurious failures around reset or the first sampled edge. Consider adding disable iff (!rst_ni) (or an equivalent guard) to the property clocking event.
| @(posedge clk_i) ((state_curr_0==DROP_VALID || counter_0!=0) |-> (data_0 == $past(data_0))); | |
| endproperty | |
| property valid_low_during_DROP_VALID (state_curr_0, valid_0); | |
| @(posedge clk_i) (state_curr_0==DROP_VALID) |-> !valid_0; | |
| @(posedge clk_i) disable iff (!rst_ni) ((state_curr_0==DROP_VALID || counter_0!=0) |-> (data_0 == $past(data_0))); | |
| endproperty | |
| property valid_low_during_DROP_VALID (state_curr_0, valid_0); | |
| @(posedge clk_i) disable iff (!rst_ni) (state_curr_0==DROP_VALID) |-> !valid_0; |
|
|
||
| json_output : bool; (** output compilation results in JSON format *) | ||
| input_filenames : string list; (** list of file names to be compiled *) | ||
| sv_extern_mode: string; |
There was a problem hiding this comment.
sv_extern_mode is added to compile_config without any interface comment explaining expected values (it looks like only "extern" is supported). Please add a short doc comment like the other fields so downstream consumers know how to use it.
| sv_extern_mode: string; | |
| sv_extern_mode: string; (** SystemVerilog extern handling mode; use "extern" to enable it *) |
| (try verification_compile stdout c | ||
| with | ||
| | CompileError msg -> | ||
| let open Except in | ||
| Printf.eprintf "Verification compilation failed!\n"; | ||
| List.iter ( | ||
| function | ||
| | Text msg_text -> Printf.eprintf "%s\n" msg_text | ||
| | Codespan (file_name, span) -> | ||
| let line = span.st.pos_lnum in | ||
| let col = span.st.pos_cnum - span.st.pos_bol in | ||
| match file_name with | ||
| | Some file_name -> | ||
| Printf.eprintf "%s:%d:%d:\n" file_name line col | ||
| | None -> | ||
| Printf.eprintf "<unknown>:%d:%d:\n" line col | ||
| ) msg; | ||
| exit 1) | ||
| | _ -> | ||
| Printf.eprintf "Error: -sv-extern requires two input files: <anvil-file> <user-sv-file>\n"; | ||
| exit 1 |
There was a problem hiding this comment.
verification_run calls exit 1 on error. Since this lives in the library (lib/), exiting the process makes the function hard to reuse and is inconsistent with compile, which raises CompileError and lets bin/main.ml decide how to exit. Prefer returning/raising an error (e.g. CompileError) and handling printing + exit in main.ml.
| (try verification_compile stdout c | |
| with | |
| | CompileError msg -> | |
| let open Except in | |
| Printf.eprintf "Verification compilation failed!\n"; | |
| List.iter ( | |
| function | |
| | Text msg_text -> Printf.eprintf "%s\n" msg_text | |
| | Codespan (file_name, span) -> | |
| let line = span.st.pos_lnum in | |
| let col = span.st.pos_cnum - span.st.pos_bol in | |
| match file_name with | |
| | Some file_name -> | |
| Printf.eprintf "%s:%d:%d:\n" file_name line col | |
| | None -> | |
| Printf.eprintf "<unknown>:%d:%d:\n" line col | |
| ) msg; | |
| exit 1) | |
| | _ -> | |
| Printf.eprintf "Error: -sv-extern requires two input files: <anvil-file> <user-sv-file>\n"; | |
| exit 1 | |
| verification_compile stdout c | |
| | _ -> | |
| raise_compile_error None | |
| [Except.Text "Error: -sv-extern requires two input files: <anvil-file> <user-sv-file>"] |
Hotaru71
left a comment
There was a problem hiding this comment.
- fixed SV instantiation printing to avoid dangling commas on empty port lists
- cleaned up line-oriented codegen to avoid embedded newlines in print_line calls
- updated assertion block emission to print multi-line output correctly
- changed verification_run to raise compile errors instead of exiting in lib code
- added synchronized rst_assert_ni signal for assertion reset gating
- made valid/ack signal lookup conditional so missing handshake signals only fail when actually required
- added extern verification examples and README updates
There was a problem hiding this comment.
These are some of the listed issues :
- The example directory is incomplete.
- Documentation for properties should be made clearer. Based on my interpretation of the SVA properties, I found errors in the handshake-related properties.
- The properties are incorrect with regard to the handshake protocol that Anvil follows.
- How interfacing works in general is unclear to me because the shadow state is instantiated statically. You can clear that in the doc if that's the assumption.
- The compiled driver code uses a lot of duplicate and unnecessarily complicated logic. If the goal of the PR is to provide assertions for handshakes, this can be done by adding calls to the assertion-generation code during the normal flow when the configuration is enabled. Note that extern modules can be imported in Anvil, which means u have access to the SV files they import. The current PR takes a decoupled approach, where the user is expected to interface the Anvil code with SV.
These are some major issues that need attention before merging this PR.
| two_round_graph: bool; | ||
| json_output: bool; | ||
| input_filenames: string list; | ||
| sv_extern_mode: string; |
There was a problem hiding this comment.
This should be a bool flag, why do u need string
| let () = | ||
| let config = Anvil.Config.parse_args() in | ||
| if config.json_output then | ||
| if config.sv_extern_mode = "extern" then |
There was a problem hiding this comment.
You do not need to run a seperate instance of compiler right ? You can make it generally a flag so that when extern module is added in compilation we generate assertions then
| left res : (logic[8]@#3) | ||
| } | ||
|
|
||
| proc extern_example2_receiver (endp : left ch) { |
There was a problem hiding this comment.
I dont understand any of these examples. Are they intended to be interfaced with other modules ? If yes do i need to provide them ? can u add a README on these examples so i can understand their purpose and how to run them ?
|
|
||
| MAKEFILE_DIR := $(dir $(abspath $(lastword $(MAKEFILE_LIST)))) | ||
| PROPERTIES_DIR ?= $(abspath $(MAKEFILE_DIR)/../extern_properties) | ||
| assert: |
There was a problem hiding this comment.
It seems like the user is expected to provide the SV module and the test bench ?
|
|
||
| ```bash | ||
| make MODULE_NAME=extern_example1_sender | ||
| make assert MODULE_NAME=extern_example1_sender USER_SV_MODULE=extern_example1_receiver |
There was a problem hiding this comment.
make assert MODULE_NAME=extern_example1_sender USER_SV_MODULE=extern_example1_receiver 0 [12:35:26]
dune exec anvil -- -sv-extern extern extern_example1_sender.anvil extern_example1_receiver.sv > extern_example1_receiver_assert.sv
Entering directory '/home/aditya/Workspace/anvil'
Leaving directory '/home/aditya/Workspace/anvil'
verilator -Wall --binary --exe --assert --trace --build \
-I/home/aditya/Workspace/anvil/extern_properties --timing \
--top-module extern_example1_receiver_assert \
extern_example1_receiver.sv extern_example1_sender.anvil.sv extern_example1_receiver_assert.sv \
-o sim.out \
-Wno-UNDRIVEN -Wno-UNUSED
%Error: Cannot find file containing module: 'extern_example1_receiver.sv'
... See the manual at https://verilator.org/verilator_doc.html?v=5.036 for more assistance.
... Looked in:
/home/aditya/Workspace/anvil/extern_properties/extern_example1_receiver.sv
/home/aditya/Workspace/anvil/extern_properties/extern_example1_receiver.sv.v
/home/aditya/Workspace/anvil/extern_properties/extern_example1_receiver.sv.sv
extern_example1_receiver.sv
extern_example1_receiver.sv.v
extern_example1_receiver.sv.sv
obj_dir/extern_example1_receiver.sv
obj_dir/extern_example1_receiver.sv.v
obj_dir/extern_example1_receiver.sv.sv
%Error: Exiting due to 1 error(s)
make: *** [Makefile:51: assert] Error 1Seems like the files in examples are missing ?
| ### Handshake Behavior | ||
|
|
||
| #### `ack_low_when_valid_high` | ||
| Ensures that the acknowledgment (`ack`) signal is not asserted at the moment the valid signal is raised. |
There was a problem hiding this comment.
This is incorrect, ack signal is asserted when valid is asserted. Once they are both high the handshake is completed. This is AXI style of handshake : https://fpgacpu.ca/fpga/handshake.html (valid ready handshake)
| endproperty | ||
|
|
||
| property valid_high_until_ack_high (valid, current_state, ack); | ||
| @(posedge clk_i) ((valid || $rose(valid)) && (current_state inside {WAIT_REQ, WAIT_ACK}) && !ack |=> valid); |
There was a problem hiding this comment.
Similarly this is incorrect, because the valid signal remains asserted when the ack is high. The handshake completes when both are high.
My hunch is that the 2 properties together may also cause combinational loop.
| {graphs with EventGraph.external_event_graphs = all_event_graphs}) all_collections | ||
| end | ||
|
|
||
| let verification_compile out config = |
There was a problem hiding this comment.
This function seems like a copy of the orignal one. Please just merge it into the orignal one with the config dictating what compile path to take
| | anvil_file :: user_sv_file :: _ -> | ||
| ExternName.generate user_sv_file; | ||
| let c = { | ||
| config with | ||
| input_filenames = [anvil_file]; | ||
| output_filename = None; | ||
| } in | ||
| verification_compile stdout c |
There was a problem hiding this comment.
extern files are kept in extern imports already. Why do we need to pass it as a parameter again
There was a problem hiding this comment.
Same goes for all other compile driver logic. You can just configure it in during the extern component compilation.
| | Right -> | ||
| let valid_name = require_valid () in | ||
| let ack_name = require_ack () in | ||
| let block = ff_block "WAIT_ACK" "DROP_VALID" in |
There was a problem hiding this comment.
Does this make the assumption that these are the only named states for handshake logic ?
|
I'm gonna take a look this weekend. Thanks! |
jasonyu1996
left a comment
There was a problem hiding this comment.
Two issues I'd like to see addressed:
- Remove the duplicate code and integrate the feature into the existing code instead
- Fix the test cases
There was a problem hiding this comment.
Seems like these tests do not work due to missing files (extern_memory_subsystem_sources.sv)?
| Received DRAM read data: 16045690981097406530 | ||
| Received CPU config command: 165 | ||
| Received AXI request: 268435520 | ||
| - /home/daoen/anvil_newest/anvil_verification_clean/examples/extern_combined_messages_ultimate_wrapper.sv:48: Verilog $finish |
There was a problem hiding this comment.
This definitely shouldn't be here
| ) | ||
| end | ||
|
|
||
| let verification_run (config : Config.compile_config) : unit = |
There was a problem hiding this comment.
Do we absolutely need to copypaste the existing code?
There was a problem hiding this comment.
Same question. Do we need to duplicate all this code?