From 370d5871f4bd04d6ccd57457827dc1dfcaf74d1e Mon Sep 17 00:00:00 2001 From: Gary Wong Date: Thu, 29 May 2025 20:42:49 -0600 Subject: [PATCH 1/4] verilog: implement SystemVerilog unique/unique0/priority if semantics. There are two elements involved: 1) Apply the relevant full_case and/or parallel_case attribute(s) to the generated AST_CASE node(s), so that the existing AST frontend and subsequent passes will generate RTLIL with appropriate behaviour. (This is handled in the parser "if_attr" non-terminal.) 2) Rearrange the AST_CASE structure when necessary. For "priority if" (i.e., full_case), this requires only ensuring that directly nested "else if" branches also inherit the full_case attribute. For "unique if" and "unique0 if" (i.e., parallel_case+full_case and parallel_case alone), there are two steps: a) Flatten the AST_CASE structure such that any direct "else if" branches are mapped to additional AST_CONDs in the parent; b) Reverse the "direction" of the test: the constant 1 (true) is provided in the AST_CASE node, and the expression(s) in the if statement(s) are given in each AST_COND. This is necessary because the constant 1, being the common factor, must occupy the shared AST_CASE position. (This is handled in the parser "TOK_IF" expansion of behavioral_stmt.) Observe that: * The generated AST has not been changed for bare "if"s (those without unique/priority). This should minimise the risk of unexpected regressions. * It is possible that the flattening described in 2) a) above might affect the behaviour of expressions with side effects in "unique if" statements (consider "unique if( a ) ...; else if( b++ ) ...": if a is true, is b incremented?). While it might be possible to provide precise semantics here, IEEE 1800-2012 12.4.2 seems to be deliberately vague ("In unique-if and unique0-if, the conditions may be evaluated and compared in any order[...] The presence of side effects in conditions may cause nondeterministic results.") and so it seems doubtful that there is benefit in Yosys providing stronger promises on the interpretation of questionable code. --- docs/source/yosys_internals/verilog.rst | 5 +-- frontends/verilog/verilog_parser.y | 44 ++++++++++++++++++------- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/docs/source/yosys_internals/verilog.rst b/docs/source/yosys_internals/verilog.rst index eef215e5d..128280c06 100644 --- a/docs/source/yosys_internals/verilog.rst +++ b/docs/source/yosys_internals/verilog.rst @@ -377,7 +377,4 @@ from SystemVerilog: - Assignments within expressions are supported. - The ``unique``, ``unique0``, and ``priority`` SystemVerilog keywords are - accepted on ``if`` and ``case`` conditionals. (Those keywords are currently - handled in the same way as their equivalent ``full_case`` and - ``parallel_case`` attributes on ``case`` statements, and checked - for syntactic validity but otherwise ignored on ``if`` statements.) + supported on ``if`` and ``case`` conditionals. diff --git a/frontends/verilog/verilog_parser.y b/frontends/verilog/verilog_parser.y index e70542a92..485b5391c 100644 --- a/frontends/verilog/verilog_parser.y +++ b/frontends/verilog/verilog_parser.y @@ -2872,16 +2872,34 @@ behavioral_stmt: ast_stack.pop_back(); } | if_attr TOK_IF '(' expr ')' { - AstNode *node = new AstNode(AST_CASE); + AstNode *node = 0; + AstNode *context = ast_stack.back(); + if (context && context->type == AST_BLOCK && context->get_bool_attribute(ID::promoted_if)) { + AstNode *outer = ast_stack[ast_stack.size() - 2]; + log_assert (outer && outer->type == AST_CASE); + if (outer->get_bool_attribute(ID::parallel_case)) { + // parallel "else if": append condition to outer "if" + node = outer; + log_assert (node->children.size()); + delete node->children.back(); + node->children.pop_back(); + } else if (outer->get_bool_attribute(ID::full_case)) + (*$1)[ID::full_case] = AstNode::mkconst_int(1, false); + } + AstNode *expr = new AstNode(AST_REDUCE_BOOL, $4); + if (!node) { + // not parallel "else if": begin new construction + node = new AstNode(AST_CASE); + append_attr(node, $1); + ast_stack.back()->children.push_back(node); + node->children.push_back(node->get_bool_attribute(ID::parallel_case) ? AstNode::mkconst_int(1, false, 1) : expr); + } AstNode *block = new AstNode(AST_BLOCK); - AstNode *cond = new AstNode(AST_COND, AstNode::mkconst_int(1, false, 1), block); + AstNode *cond = new AstNode(AST_COND, node->get_bool_attribute(ID::parallel_case) ? expr : AstNode::mkconst_int(1, false, 1), block); SET_AST_NODE_LOC(cond, @4, @4); - ast_stack.back()->children.push_back(node); - node->children.push_back(new AstNode(AST_REDUCE_BOOL, $4)); node->children.push_back(cond); ast_stack.push_back(node); ast_stack.push_back(block); - append_attr(node, $1); } behavioral_stmt { SET_AST_NODE_LOC(ast_stack.back(), @7, @7); } optional_else { @@ -2907,21 +2925,25 @@ if_attr: } | attr TOK_UNIQUE0 { AstNode *context = ast_stack.back(); - if( context && context->type == AST_BLOCK && context->get_bool_attribute(ID::promoted_if) ) + if (context && context->type == AST_BLOCK && context->get_bool_attribute(ID::promoted_if)) frontend_verilog_yyerror("unique0 keyword cannot be used for 'else if' branch."); - $$ = $1; // accept unique0 keyword, but ignore it for now + (*$1)[ID::parallel_case] = AstNode::mkconst_int(1, false); + $$ = $1; } | attr TOK_PRIORITY { AstNode *context = ast_stack.back(); - if( context && context->type == AST_BLOCK && context->get_bool_attribute(ID::promoted_if) ) + if (context && context->type == AST_BLOCK && context->get_bool_attribute(ID::promoted_if)) frontend_verilog_yyerror("priority keyword cannot be used for 'else if' branch."); - $$ = $1; // accept priority keyword, but ignore it for now + (*$1)[ID::full_case] = AstNode::mkconst_int(1, false); + $$ = $1; } | attr TOK_UNIQUE { AstNode *context = ast_stack.back(); - if( context && context->type == AST_BLOCK && context->get_bool_attribute(ID::promoted_if) ) + if (context && context->type == AST_BLOCK && context->get_bool_attribute(ID::promoted_if)) frontend_verilog_yyerror("unique keyword cannot be used for 'else if' branch."); - $$ = $1; // accept unique keyword, but ignore it for now + (*$1)[ID::full_case] = AstNode::mkconst_int(1, false); + (*$1)[ID::parallel_case] = AstNode::mkconst_int(1, false); + $$ = $1; }; case_attr: From 7b09dc31af5ca7319ec41b2c1e91dde44a5dce42 Mon Sep 17 00:00:00 2001 From: Gary Wong Date: Thu, 29 May 2025 20:43:41 -0600 Subject: [PATCH 2/4] tests: add cases covering full_case and parallel_case semantics This is @KrystalDelusion's suggestion in PR #5141 to verify sensible implementation of all 4 possible full_case/parallel_case combinations. (Also including two similar tests to check the Verilog frontend applies the correct attributes when given SystemVerilog priority/unique case and if statements.) --- tests/proc/case_attr.ys | 57 +++++++++++++++++++++++++++ tests/verilog/unique_priority_case.ys | 54 +++++++++++++++++++++++++ tests/verilog/unique_priority_if.ys | 54 +++++++++++++++++++++++++ 3 files changed, 165 insertions(+) create mode 100644 tests/proc/case_attr.ys create mode 100644 tests/verilog/unique_priority_case.ys create mode 100644 tests/verilog/unique_priority_if.ys diff --git a/tests/proc/case_attr.ys b/tests/proc/case_attr.ys new file mode 100644 index 000000000..63be79fe5 --- /dev/null +++ b/tests/proc/case_attr.ys @@ -0,0 +1,57 @@ +read_verilog -sv << EOF +module top (input A, B, C, D, E, F, output reg W, X, Y, Z); + always @* begin + W = F; + (* full_case *) + case (C) + A: W = D; + B: W = E; + endcase + end + + always @* begin + X = F; + case (C) + A: X = D; + B: X = E; + endcase + end + + always @* begin + Y = F; + (* full_case, parallel_case *) + case (C) + A: Y = D; + B: Y = E; + endcase + end + + always @* begin + Z = F; + (* parallel_case *) + case (C) + A: Z = D; + B: Z = E; + endcase + end +endmodule +EOF +prep +# For the ones which use full_case, the F signal shouldn't be included in +# the input cone of W and Y. +select -set full o:W o:Y %u %ci* +select -assert-none i:F @full %i +select -assert-count 1 o:X %ci* i:F %i +select -assert-count 1 o:Z %ci* i:F %i + +# And for parallel_case there should be 1 $pmux compared to the 2 $mux +# cells without. +select -assert-none o:W %ci* t:$pmux %i +select -assert-none o:X %ci* t:$pmux %i +select -assert-count 1 o:Y %ci* t:$pmux %i +select -assert-count 1 o:Z %ci* t:$pmux %i + +select -assert-count 2 o:W %ci* t:$mux %i +select -assert-count 2 o:X %ci* t:$mux %i +select -assert-none o:Y %ci* t:$mux %i +select -assert-none o:Z %ci* t:$mux %i diff --git a/tests/verilog/unique_priority_case.ys b/tests/verilog/unique_priority_case.ys new file mode 100644 index 000000000..1f87fb19c --- /dev/null +++ b/tests/verilog/unique_priority_case.ys @@ -0,0 +1,54 @@ +read_verilog -sv << EOF +module top (input A, B, C, D, E, F, output reg W, X, Y, Z); + always_comb begin + W = F; + priority case (C) + A: W = D; + B: W = E; + endcase + end + + always_comb begin + X = F; + case (C) + A: X = D; + B: X = E; + endcase + end + + always_comb begin + Y = F; + unique case (C) + A: Y = D; + B: Y = E; + endcase + end + + always_comb begin + Z = F; + unique0 case (C) + A: Z = D; + B: Z = E; + endcase + end +endmodule +EOF +prep +# For the ones which use priority/unique, the F signal shouldn't be included in +# the input cone of W and Y. +select -set full o:W o:Y %u %ci* +select -assert-none i:F @full %i +select -assert-count 1 o:X %ci* i:F %i +select -assert-count 1 o:Z %ci* i:F %i + +# And for unique/unique0 there should be 1 $pmux compared to the 2 $mux +# cells without. +select -assert-none o:W %ci* t:$pmux %i +select -assert-none o:X %ci* t:$pmux %i +select -assert-count 1 o:Y %ci* t:$pmux %i +select -assert-count 1 o:Z %ci* t:$pmux %i + +select -assert-count 2 o:W %ci* t:$mux %i +select -assert-count 2 o:X %ci* t:$mux %i +select -assert-none o:Y %ci* t:$mux %i +select -assert-none o:Z %ci* t:$mux %i diff --git a/tests/verilog/unique_priority_if.ys b/tests/verilog/unique_priority_if.ys new file mode 100644 index 000000000..a6053e809 --- /dev/null +++ b/tests/verilog/unique_priority_if.ys @@ -0,0 +1,54 @@ +read_verilog -sv << EOF +module top (input A, B, C, D, E, F, output reg W, X, Y, Z); + always_comb begin + W = F; + priority if (C == A) + W = D; + else if (C == B) + W = E; + end + + always_comb begin + X = F; + if (C == A) + X = D; + else if (C == B) + X = E; + end + + always_comb begin + Y = F; + unique if (C == A) + Y = D; + else if (C == B) + Y = E; + end + + always_comb begin + Z = F; + unique0 if (C == A) + Z = D; + else if (C == B) + Z = E; + end +endmodule +EOF +prep +# For the ones which use priority/unique, the F signal shouldn't be included in +# the input cone of W and Y. +select -set full o:W o:Y %u %ci* +select -assert-none i:F @full %i +select -assert-count 1 o:X %ci* i:F %i +select -assert-count 1 o:Z %ci* i:F %i + +# And for unique/unique0 there should be 1 $pmux compared to the 2 $mux +# cells without. +select -assert-none o:W %ci* t:$pmux %i +select -assert-none o:X %ci* t:$pmux %i +select -assert-count 1 o:Y %ci* t:$pmux %i +select -assert-count 1 o:Z %ci* t:$pmux %i + +select -assert-count 2 o:W %ci* t:$mux %i +select -assert-count 2 o:X %ci* t:$mux %i +select -assert-none o:Y %ci* t:$mux %i +select -assert-none o:Z %ci* t:$mux %i From 62660b221f7e106dd3d966775a49f96fe65e2248 Mon Sep 17 00:00:00 2001 From: Gary Wong Date: Fri, 30 May 2025 21:13:20 -0600 Subject: [PATCH 3/4] docs: restore and update the note about if/case attributes. --- docs/source/yosys_internals/verilog.rst | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/source/yosys_internals/verilog.rst b/docs/source/yosys_internals/verilog.rst index 128280c06..0039aaab7 100644 --- a/docs/source/yosys_internals/verilog.rst +++ b/docs/source/yosys_internals/verilog.rst @@ -377,4 +377,7 @@ from SystemVerilog: - Assignments within expressions are supported. - The ``unique``, ``unique0``, and ``priority`` SystemVerilog keywords are - supported on ``if`` and ``case`` conditionals. + supported on ``if`` and ``case`` conditionals. (The Verilog frontend + will process conditionals using these keywords by annotating their + representation with the appropriate ``full_case`` and/or ``parallel_case`` + attributes, which are described above.) From 10bb0f472fd2ccd6e28b63e20189ce1922ec7ceb Mon Sep 17 00:00:00 2001 From: Gary Wong Date: Fri, 30 May 2025 21:43:33 -0600 Subject: [PATCH 4/4] docs: mention related effects for multiplexers in the cell library. --- docs/source/cell/word_mux.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/source/cell/word_mux.rst b/docs/source/cell/word_mux.rst index 3eca310f3..234d1016b 100644 --- a/docs/source/cell/word_mux.rst +++ b/docs/source/cell/word_mux.rst @@ -24,8 +24,8 @@ are zero, the value from ``A`` input is sent to the output. If the :math:`n`\ 'th bit from ``S`` is set, the value :math:`n`\ 'th ``WIDTH`` bits wide slice of the ``B`` input is sent to the output. When more than one bit from ``S`` is set the output is undefined. Cells of this type are used to model "parallel cases" -(defined by using the ``parallel_case`` attribute or detected by an -optimization). +(defined by using the ``parallel_case`` attribute, the ``unique`` or ``unique0`` +SystemVerilog keywords, or detected by an optimization). The `$tribuf` cell is used to implement tristate logic. Cells of this type have a ``WIDTH`` parameter and inputs ``A`` and ``EN`` and an output ``Y``. The ``A``