From c7cd70b103b1ceb9e32c43fb70295887ab1ee2e4 Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Wed, 23 Apr 2025 18:57:43 +0200 Subject: [PATCH 1/6] check: promote some problems to errors by default, add -permissive --- kernel/log.cc | 10 +++++++ kernel/log.h | 1 + passes/cmds/check.cc | 67 +++++++++++++++++++++++++++++++------------- 3 files changed, 58 insertions(+), 20 deletions(-) diff --git a/kernel/log.cc b/kernel/log.cc index fabbe09fd..a6cfb9712 100644 --- a/kernel/log.cc +++ b/kernel/log.cc @@ -400,6 +400,16 @@ void log_file_error(const string &filename, int lineno, logv_file_error(filename, lineno, format, ap); } +std::string fmt(const char *format, ...) +{ + std::string s; + va_list ap; + va_start(ap, format); + s = vstringf(format, ap); + va_end(ap); + return s; +} + void log(const char *format, ...) { va_list ap; diff --git a/kernel/log.h b/kernel/log.h index e26ef072c..4dc161559 100644 --- a/kernel/log.h +++ b/kernel/log.h @@ -126,6 +126,7 @@ void logv_warning_noprefix(const char *format, va_list ap); [[noreturn]] void logv_error(const char *format, va_list ap); [[noreturn]] void logv_file_error(const string &filename, int lineno, const char *format, va_list ap); +std::string fmt(const char *format, ...) YS_ATTRIBUTE(format(printf, 1, 2)); void log(const char *format, ...) YS_ATTRIBUTE(format(printf, 1, 2)); void log_header(RTLIL::Design *design, const char *format, ...) YS_ATTRIBUTE(format(printf, 2, 3)); void log_warning(const char *format, ...) YS_ATTRIBUTE(format(printf, 1, 2)); diff --git a/passes/cmds/check.cc b/passes/cmds/check.cc index 83fe781a0..07d02b492 100644 --- a/passes/cmds/check.cc +++ b/passes/cmds/check.cc @@ -27,6 +27,11 @@ USING_YOSYS_NAMESPACE PRIVATE_NAMESPACE_BEGIN struct CheckPass : public Pass { + enum Mode { + Permissive, + Smart, + Assert, + }; CheckPass() : Pass("check", "check for obvious problems in the design") { } void help() override { @@ -59,6 +64,10 @@ struct CheckPass : public Pass { log(" -assert\n"); log(" produce a runtime error if any problems are found in the current design\n"); log("\n"); + log(" -permissive\n"); + log(" treat even severe problems as just warnings.\n"); + log(" Used to be the default behavior\n"); + log("\n"); log(" -force-detailed-loop-check\n"); log(" for the detection of combinatorial loops, use a detailed connectivity\n"); log(" model for all internal cells for which it is available. This disables\n"); @@ -68,14 +77,25 @@ struct CheckPass : public Pass { } void execute(std::vector args, RTLIL::Design *design) override { + // Number of all problems (warnings and errors) int counter = 0; bool noinit = false; bool initdrv = false; bool mapped = false; bool allow_tbuf = false; - bool assert_mode = false; bool force_detailed_loop_check = false; bool suggest_detail = false; + Mode mode = Mode::Smart; + // log_error always terminates and it's a huge hassle to refactor + std::vector errors; + std::function bad = [&errors, &counter](std::string message) { + counter++; + errors.push_back(message); + }; + std::function warn = [&counter](std::string message) { + counter++; + log_warning("%s", message.c_str()); + }; size_t argidx; for (argidx = 1; argidx < args.size(); argidx++) { @@ -95,8 +115,12 @@ struct CheckPass : public Pass { allow_tbuf = true; continue; } + if (args[argidx] == "-permissive") { + mode = Mode::Permissive; + continue; + } if (args[argidx] == "-assert") { - assert_mode = true; + mode = Mode::Assert; continue; } if (args[argidx] == "-force-detailed-loop-check") { @@ -109,6 +133,11 @@ struct CheckPass : public Pass { log_header(design, "Executing CHECK pass (checking for obvious problems).\n"); + if (mode == Mode::Permissive) + bad = warn; + if (mode == Mode::Assert) + warn = bad; + for (auto module : design->selected_whole_modules_warn()) { log("Checking module %s...\n", log_id(module)); @@ -246,8 +275,7 @@ struct CheckPass : public Pass { { if (mapped && cell->type.begins_with("$") && design->module(cell->type) == nullptr) { if (allow_tbuf && cell->type == ID($_TBUF_)) goto cell_allowed; - log_warning("Cell %s.%s is an unmapped internal cell of type %s.\n", log_id(module), log_id(cell), log_id(cell->type)); - counter++; + warn(fmt("Cell %s.%s is an unmapped internal cell of type %s.\n", log_id(module), log_id(cell), log_id(cell->type))); cell_allowed:; } @@ -299,8 +327,7 @@ struct CheckPass : public Pass { if (initval[i] == State::S0 || initval[i] == State::S1) init_bits.insert(sigmap(SigBit(wire, i))); if (noinit) { - log_warning("Wire %s.%s has an unprocessed 'init' attribute.\n", log_id(module), log_id(wire)); - counter++; + warn(fmt("Wire %s.%s has an unprocessed 'init' attribute.\n", log_id(module), log_id(wire))); } } } @@ -310,8 +337,7 @@ struct CheckPass : public Pass { string message = stringf("Drivers conflicting with a constant %s driver:\n", log_signal(state)); for (auto str : wire_drivers[state]) message += stringf(" %s\n", str.c_str()); - log_warning("%s", message.c_str()); - counter++; + bad(message); } for (auto it : wire_drivers) @@ -319,14 +345,12 @@ struct CheckPass : public Pass { string message = stringf("multiple conflicting drivers for %s.%s:\n", log_id(module), log_signal(it.first)); for (auto str : it.second) message += stringf(" %s\n", str.c_str()); - log_warning("%s", message.c_str()); - counter++; + bad(message); } for (auto bit : used_wires) if (!wire_drivers.count(bit)) { - log_warning("Wire %s.%s is used but has no driver.\n", log_id(module), log_signal(bit)); - counter++; + warn(fmt("Wire %s.%s is used but has no driver.\n", log_id(module), log_signal(bit))); } topo.sort(); @@ -386,7 +410,7 @@ struct CheckPass : public Pass { message += stringf(" cell %s (%s)%s\n", log_id(driver), log_id(driver->type), driver_src.c_str()); - if (!coarsened_cells.count(driver)) { + if (!coarsened_cells.count(driver)) { MatchingEdgePrinter printer(message, sigmap, prev, bit); printer.add_edges_from_cell(driver); } else { @@ -400,13 +424,12 @@ struct CheckPass : public Pass { std::string src_attr = wire->get_src_attribute(); wire_src = stringf(" source: %s", src_attr.c_str()); } - message += stringf(" wire %s%s\n", log_signal(SigBit(wire, pair.second)), wire_src.c_str()); + message += stringf(" wire %s%s\n", log_signal(SigBit(wire, pair.second)), wire_src.c_str()); } prev = bit; } - log_warning("%s", message.c_str()); - counter++; + bad(message.c_str()); } if (initdrv) @@ -424,8 +447,7 @@ struct CheckPass : public Pass { init_sig.sort_and_unify(); for (auto chunk : init_sig.chunks()) { - log_warning("Wire %s.%s has 'init' attribute and is not driven by an FF cell.\n", log_id(module), log_signal(chunk)); - counter++; + warn(fmt("Wire %s.%s has 'init' attribute and is not driven by an FF cell.\n", log_id(module), log_signal(chunk))); } } } @@ -435,8 +457,13 @@ struct CheckPass : public Pass { if (suggest_detail) log("Consider re-running with '-force-detailed-loop-check' to rule out false positives.\n"); - if (assert_mode && counter > 0) - log_error("Found %d problems in 'check -assert'.\n", counter); + if (errors.size()) { + std::string err_message; + for (auto error : errors) + err_message += error + "\n"; + err_message += fmt("Found %zu severe problems in 'check'.\n", errors.size()); + log_error("%s", err_message.c_str()); + } } } CheckPass; From 0601260bf228f14dbd383e188d3cca4c2fd0a660 Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Thu, 8 May 2025 14:37:03 +0200 Subject: [PATCH 2/6] check: add scratchpad check.permissive and use it --- passes/cmds/check.cc | 2 ++ tests/various/dynamic_part_select.ys | 1 + 2 files changed, 3 insertions(+) diff --git a/passes/cmds/check.cc b/passes/cmds/check.cc index 07d02b492..99e81c549 100644 --- a/passes/cmds/check.cc +++ b/passes/cmds/check.cc @@ -86,6 +86,8 @@ struct CheckPass : public Pass { bool force_detailed_loop_check = false; bool suggest_detail = false; Mode mode = Mode::Smart; + if (design->scratchpad_get_bool("check.permissive")) + mode = Mode::Permissive; // log_error always terminates and it's a huge hassle to refactor std::vector errors; std::function bad = [&errors, &counter](std::string message) { diff --git a/tests/various/dynamic_part_select.ys b/tests/various/dynamic_part_select.ys index 9e303b9db..3037fdfdc 100644 --- a/tests/various/dynamic_part_select.ys +++ b/tests/various/dynamic_part_select.ys @@ -142,6 +142,7 @@ design -copy-from gate -as gate gate miter -equiv -make_assert -make_outcmp -flatten gold gate equiv sat -prove-asserts -show-public -verify -set-init-zero equiv +scratchpad -set check.permissive true ### ## Part select with obvious latch, expected to fail due comparison with old shift&mask AST transformation design -reset From 56d562299a9b140e929f3114d2b5da05de684bab Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Wed, 7 May 2025 15:26:09 +0200 Subject: [PATCH 3/6] remove invalid tests, use scratchpad variable check.permissive --- tests/asicworld/code_hdl_models_d_ff_gates.v | 29 ------------------- .../asicworld/code_hdl_models_d_latch_gates.v | 15 ---------- tests/hana/test_intermout.v | 25 ---------------- tests/memories/wide_all.v | 2 +- 4 files changed, 1 insertion(+), 70 deletions(-) delete mode 100644 tests/asicworld/code_hdl_models_d_ff_gates.v delete mode 100644 tests/asicworld/code_hdl_models_d_latch_gates.v diff --git a/tests/asicworld/code_hdl_models_d_ff_gates.v b/tests/asicworld/code_hdl_models_d_ff_gates.v deleted file mode 100644 index 8706f154c..000000000 --- a/tests/asicworld/code_hdl_models_d_ff_gates.v +++ /dev/null @@ -1,29 +0,0 @@ -module d_ff_gates(d,clk,q,q_bar); -input d,clk; -output q, q_bar; - -wire n1,n2,n3,q_bar_n; -wire cn,dn,n4,n5,n6; - -// First Latch -not (n1,d); - -nand (n2,d,clk); -nand (n3,n1,clk); - -nand (dn,q_bar_n,n2); -nand (q_bar_n,dn,n3); - -// Second Latch -not (cn,clk); - -not (n4,dn); - -nand (n5,dn,cn); -nand (n6,n4,cn); - -nand (q,q_bar,n5); -nand (q_bar,q,n6); - - -endmodule diff --git a/tests/asicworld/code_hdl_models_d_latch_gates.v b/tests/asicworld/code_hdl_models_d_latch_gates.v deleted file mode 100644 index 3f5f6b2bb..000000000 --- a/tests/asicworld/code_hdl_models_d_latch_gates.v +++ /dev/null @@ -1,15 +0,0 @@ -module d_latch_gates(d,clk,q,q_bar); -input d,clk; -output q, q_bar; - -wire n1,n2,n3; - -not (n1,d); - -nand (n2,d,clk); -nand (n3,n1,clk); - -nand (q,q_bar,n2); -nand (q_bar,q,n3); - -endmodule diff --git a/tests/hana/test_intermout.v b/tests/hana/test_intermout.v index 88b91ee4d..4c231ab3a 100644 --- a/tests/hana/test_intermout.v +++ b/tests/hana/test_intermout.v @@ -182,31 +182,6 @@ assign w2 = w1; assign out = w2; endmodule - -// test_intermout_bufrm_7_test.v -module f15_test(in1, in2, out); -input in1, in2; -output out; -// Y with cluster of f15_mybuf instances at the junction - -wire w1, w2, w3, w4, w5, w6, w7, w8, w9, w10; -assign w1 = in1; -assign w2 = w1; -assign w5 = in2; -assign w6 = w5; -assign w10 = w9; -assign out = w10; - -f15_mybuf _f15_mybuf0(w2, w3); -f15_mybuf _f15_mybuf1(w3, w4); - -f15_mybuf _f15_mybuf2(w6, w7); -f15_mybuf _f15_mybuf3(w7, w4); - -f15_mybuf _f15_mybuf4(w4, w8); -f15_mybuf _f15_mybuf5(w8, w9); -endmodule - module f15_mybuf(in, out); input in; output out; diff --git a/tests/memories/wide_all.v b/tests/memories/wide_all.v index f7bc3e5ce..1516b26fd 100644 --- a/tests/memories/wide_all.v +++ b/tests/memories/wide_all.v @@ -14,7 +14,7 @@ module test( reg [7:0] mem[3:254]; assign rd[7:0] = mem[{ra, 1'b0}]; -assign rd[15:0] = mem[{ra, 1'b1}]; +assign rd[15:8] = mem[{ra, 1'b1}]; initial begin mem[5] = 8'h12; From 051c00e7dbc761cc732b43177b3af85fd86e9114 Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Wed, 7 May 2025 17:52:31 +0200 Subject: [PATCH 4/6] use scratchpad variable check.permissive --- tests/simple_abc9/abc9.v | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/simple_abc9/abc9.v b/tests/simple_abc9/abc9.v index fba089b1f..abe1f6bfd 100644 --- a/tests/simple_abc9/abc9.v +++ b/tests/simple_abc9/abc9.v @@ -47,6 +47,7 @@ module abc9_test008_sub(input a, output b); assign b = ~a; endmodule +scratchpad -set check.permissive true module abc9_test009(inout io, input oe); reg latch; always @(io or oe) @@ -80,7 +81,7 @@ assign io = oe ? ~latch : 8'bz; endmodule module abc9_test013(inout [3:0] io, input oe); -reg [3:0] latch; +reg [7:0] latch; always @(io or oe) if (!oe) latch[3:0] <= io[3:0]; @@ -104,6 +105,7 @@ always @(io or oe) assign io[3:0] = oe ? ~latch[3:0] : 4'bz; assign io[7:4] = !oe ? {latch[4], latch[7:3]} : 4'bz; endmodule +scratchpad -set check.permissive false module abc9_test015(input a, output b, input c); assign b = ~a; From d3894caf2597438527ad4073f5c8af5c37abe3a1 Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Thu, 8 May 2025 12:35:25 +0200 Subject: [PATCH 5/6] logger: allow multiple regexes for -expect error --- passes/cmds/logger.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/passes/cmds/logger.cc b/passes/cmds/logger.cc index 3277e1608..8b2b739da 100644 --- a/passes/cmds/logger.cc +++ b/passes/cmds/logger.cc @@ -153,8 +153,6 @@ struct LoggerPass : public Pass { std::string type = args[++argidx]; if (type!="error" && type!="warning" && type!="log") log_cmd_error("Expect command require type to be 'log', 'warning' or 'error' !\n"); - if (type=="error" && log_expect_error.size()>0) - log_cmd_error("Only single error message can be expected !\n"); std::string pattern = args[++argidx]; if (pattern.front() == '\"' && pattern.back() == '\"') pattern = pattern.substr(1, pattern.size() - 2); int count = atoi(args[++argidx].c_str()); From 068dd77a14cad3ee3cf8555c77be44707c69b26a Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Thu, 8 May 2025 12:37:10 +0200 Subject: [PATCH 6/6] check: fix up tests --- tests/various/check_2.ys | 4 +--- tests/various/check_3.ys | 5 ++--- tests/various/check_4.ys | 2 +- tests/various/constant_drive_conflict.ys | 6 +++--- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/tests/various/check_2.ys b/tests/various/check_2.ys index b63ea6837..0f73b3469 100644 --- a/tests/various/check_2.ys +++ b/tests/various/check_2.ys @@ -11,7 +11,5 @@ module top(input clk, input a, input b, output [9:0] x); endmodule EOF hierarchy -top top +logger -expect error "found logic loop in module top:" 1 prep -logger -expect warning "found logic loop in module top:" 1 -logger -expect error "Found 1 problems in 'check -assert'" 1 -check -assert diff --git a/tests/various/check_3.ys b/tests/various/check_3.ys index 14915ab8d..102880cd0 100644 --- a/tests/various/check_3.ys +++ b/tests/various/check_3.ys @@ -14,7 +14,6 @@ module pingpong(input wire [1:0] x, output wire [3:0] y1, output wire [3:0] y2); endmodule EOF hierarchy -top pingpong +logger -expect error "found logic loop in module pingpong:" 1 +logger -expect error "Found [0-9]+ severe problems in 'check'" 1 prep -logger -nowarn "found logic loop in module pingpong:" -logger -expect error "Found [0-9]+ problems in 'check -assert'" 1 -check -assert diff --git a/tests/various/check_4.ys b/tests/various/check_4.ys index 010cd01fd..1f72928c2 100644 --- a/tests/various/check_4.ys +++ b/tests/various/check_4.ys @@ -25,5 +25,5 @@ opt -keepdc memory_dff opt_clean logger -nowarn "found logic loop in module pingpong:" -logger -expect error "Found [0-9]+ problems in 'check -assert'" 1 +logger -expect error "Found [0-9]+ severe problems in 'check'" 1 check -assert diff --git a/tests/various/constant_drive_conflict.ys b/tests/various/constant_drive_conflict.ys index 1246cbcae..e528e1ab0 100644 --- a/tests/various/constant_drive_conflict.ys +++ b/tests/various/constant_drive_conflict.ys @@ -8,7 +8,7 @@ EOT hierarchy -top top; proc -logger -expect warning "Drivers conflicting with a constant" 1 +logger -expect error "Drivers conflicting with a constant" 1 logger -expect log "Found and reported 1 problems." 1 check logger -check-expected @@ -26,7 +26,7 @@ EOT hierarchy -top top; proc -logger -expect warning "Drivers conflicting with a constant" 1 +logger -expect error "Drivers conflicting with a constant" 1 logger -expect log "Found and reported 1 problems." 1 check logger -check-expected @@ -45,7 +45,7 @@ EOT hierarchy -top top -logger -expect warning "Drivers conflicting with a constant" 1 +logger -expect error "Drivers conflicting with a constant" 1 logger -expect log "Found and reported 1 problems." 1 check logger -check-expected