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

sv: support assignments within expressions #3920

Merged
merged 3 commits into from
Sep 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ List of major changes and improvements between releases
Yosys 0.33 .. Yosys 0.34-dev
--------------------------

* SystemVerilog
- Added support for assignments within expressions, e.g., `x[y++] = z;` or
`x = (y *= 2) - 1;`.

Yosys 0.32 .. Yosys 0.33
--------------------------
* Various
Expand Down
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,8 @@ from SystemVerilog:
- SystemVerilog interfaces (SVIs) are supported. Modports for specifying whether
ports are inputs or outputs are supported.

- Assignments within expressions are supported.


Building the documentation
==========================
Expand Down
113 changes: 90 additions & 23 deletions frontends/verilog/verilog_parser.y
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,61 @@ static void rewriteGenForDeclInit(AstNode *loop)
substitute(incr);
}

static void ensureAsgnExprAllowed()
{
if (!sv_mode)
frontend_verilog_yyerror("Assignments within expressions are only supported in SystemVerilog mode.");
if (ast_stack.back()->type != AST_BLOCK)
frontend_verilog_yyerror("Assignments within expressions are only permitted within procedures.");
}

// add a pre/post-increment/decrement statement
static const AstNode *addIncOrDecStmt(AstNode *lhs, dict<IdString, AstNode*> *attr, AST::AstNodeType op, YYLTYPE begin, YYLTYPE end)
{
AstNode *one = AstNode::mkconst_int(1, true);
AstNode *rhs = new AstNode(op, lhs->clone(), one);
AstNode *stmt = new AstNode(AST_ASSIGN_EQ, lhs, rhs);
SET_AST_NODE_LOC(stmt, begin, end);
if (attr != nullptr)
append_attr(stmt, attr);
ast_stack.back()->children.push_back(stmt);
return stmt;
}

// create a pre/post-increment/decrement expression, and add the corresponding statement
static AstNode *addIncOrDecExpr(AstNode *lhs, dict<IdString, AstNode*> *attr, AST::AstNodeType op, YYLTYPE begin, YYLTYPE end, bool undo)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to recover the sizing information here, or do something other than the plus one minus one dance (maybe an assignment to a dummy wire?). Consider the following:

  w = {1, r++};

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great catch! I should have set the width of minus_one explicitly to prevent it from affecting the width of the expression. This logic seems simpler than the alternatives I considered: A) adding a temporary wire; or B) actually doing the increment afterwards. That said, my preference here likely comes from having done it the same way in sv2v.

ensureAsgnExprAllowed();
const AstNode *stmt = addIncOrDecStmt(lhs, attr, op, begin, end);
log_assert(stmt->type == AST_ASSIGN_EQ);
AstNode *expr = stmt->children[0]->clone();
if (undo) {
AstNode *minus_one = AstNode::mkconst_int(-1, true);
expr = new AstNode(op, expr, minus_one);
}
SET_AST_NODE_LOC(expr, begin, end);
return expr;
}

// add a binary operator assignment statement, e.g., a += b
static const AstNode *addAsgnBinopStmt(dict<IdString, AstNode*> *attr, AstNode *lhs, AST::AstNodeType op, AstNode *rhs, YYLTYPE begin, YYLTYPE end)
{
SET_AST_NODE_LOC(rhs, end, end);
if (op == AST_SHIFT_LEFT || op == AST_SHIFT_RIGHT ||
op == AST_SHIFT_SLEFT || op == AST_SHIFT_SRIGHT) {
rhs = new AstNode(AST_TO_UNSIGNED, rhs);
SET_AST_NODE_LOC(rhs, end, end);
}
rhs = new AstNode(op, lhs->clone(), rhs);
AstNode *stmt = new AstNode(AST_ASSIGN_EQ, lhs, rhs);
SET_AST_NODE_LOC(rhs, begin, end);
SET_AST_NODE_LOC(stmt, begin, end);
ast_stack.back()->children.push_back(stmt);
if (attr != nullptr)
append_attr(stmt, attr);
return lhs;
}

%}

%define api.prefix {frontend_verilog_yy}
Expand Down Expand Up @@ -358,7 +413,7 @@ static void rewriteGenForDeclInit(AstNode *loop)
%type <integer> integer_atom_type integer_vector_type
%type <al> attr case_attr
%type <ast> struct_union
%type <ast_node_type> asgn_binop
%type <ast_node_type> asgn_binop inc_or_dec_op
%type <ast> genvar_identifier

%type <specify_target_ptr> specify_target
Expand Down Expand Up @@ -2610,17 +2665,15 @@ simple_behavioral_stmt:
SET_AST_NODE_LOC(node, @2, @5);
append_attr(node, $1);
} |
attr lvalue TOK_INCREMENT {
AstNode *node = new AstNode(AST_ASSIGN_EQ, $2, new AstNode(AST_ADD, $2->clone(), AstNode::mkconst_int(1, true)));
ast_stack.back()->children.push_back(node);
SET_AST_NODE_LOC(node, @2, @3);
append_attr(node, $1);
attr lvalue attr inc_or_dec_op {
// The position 1 attr to avoid shift/reduce conflicts with the
// other productions. We reject attributes in that position.
if (!$1->empty())
frontend_verilog_yyerror("Attributes are not allowed on this size of the lvalue in an inc_or_dec_expression!");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell these attributes would be allowed by the standard, since attributes can be prefix to a statement and this is a self-standing statement, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I'll pushing a fix for this shortly.

addIncOrDecStmt($2, $3, $4, @1, @4);
} |
attr lvalue TOK_DECREMENT {
AstNode *node = new AstNode(AST_ASSIGN_EQ, $2, new AstNode(AST_SUB, $2->clone(), AstNode::mkconst_int(1, true)));
ast_stack.back()->children.push_back(node);
SET_AST_NODE_LOC(node, @2, @3);
append_attr(node, $1);
inc_or_dec_op attr lvalue {
addIncOrDecStmt($3, $2, $1, @1, @3);
} |
attr lvalue OP_LE delay expr {
AstNode *node = new AstNode(AST_ASSIGN_LE, $2, $5);
Expand All @@ -2629,18 +2682,7 @@ simple_behavioral_stmt:
append_attr(node, $1);
} |
attr lvalue asgn_binop delay expr {
AstNode *expr_node = $5;
if ($3 == AST_SHIFT_LEFT || $3 == AST_SHIFT_RIGHT ||
$3 == AST_SHIFT_SLEFT || $3 == AST_SHIFT_SRIGHT) {
expr_node = new AstNode(AST_TO_UNSIGNED, expr_node);
SET_AST_NODE_LOC(expr_node, @5, @5);
}
AstNode *op_node = new AstNode($3, $2->clone(), expr_node);
AstNode *node = new AstNode(AST_ASSIGN_EQ, $2, op_node);
SET_AST_NODE_LOC(op_node, @2, @5);
SET_AST_NODE_LOC(node, @2, @5);
ast_stack.back()->children.push_back(node);
append_attr(node, $1);
addAsgnBinopStmt($1, $2, $3, $5, @2, @5);
};

asgn_binop:
Expand All @@ -2657,6 +2699,12 @@ asgn_binop:
TOK_SSHL_ASSIGN { $$ = AST_SHIFT_SLEFT; } |
TOK_SSHR_ASSIGN { $$ = AST_SHIFT_SRIGHT; } ;

inc_or_dec_op:
// NOTE: These should only be permitted in SV mode, but Yosys has
// allowed them in all modes since support for them was added in 2017.
TOK_INCREMENT { $$ = AST_ADD; } |
TOK_DECREMENT { $$ = AST_SUB; } ;

for_initialization:
TOK_ID '=' expr {
AstNode *ident = new AstNode(AST_IDENTIFIER);
Expand Down Expand Up @@ -3149,6 +3197,14 @@ expr:
$$->children.push_back($6);
SET_AST_NODE_LOC($$, @1, @$);
append_attr($$, $3);
} |
inc_or_dec_op attr rvalue {
$$ = addIncOrDecExpr($3, $2, $1, @1, @3, false);
} |
// TODO: Attributes are allowed in the middle here, but they create some
// non-trivial conflicts that don't seem worth solving for now.
rvalue inc_or_dec_op {
$$ = addIncOrDecExpr($1, nullptr, $2, @1, @2, true);
};

basic_expr:
Expand Down Expand Up @@ -3436,6 +3492,17 @@ basic_expr:
frontend_verilog_yyerror("Static cast is only supported in SystemVerilog mode.");
$$ = new AstNode(AST_CAST_SIZE, $1, $4);
SET_AST_NODE_LOC($$, @1, @4);
} |
'(' expr '=' expr ')' {
ensureAsgnExprAllowed();
AstNode *node = new AstNode(AST_ASSIGN_EQ, $2, $4);
ast_stack.back()->children.push_back(node);
SET_AST_NODE_LOC(node, @2, @4);
$$ = $2->clone();
} |
'(' expr asgn_binop expr ')' {
ensureAsgnExprAllowed();
$$ = addAsgnBinopStmt(nullptr, $2, $3, $4, @2, @4)-> clone();
};

concat_list:
Expand Down
60 changes: 60 additions & 0 deletions tests/verilog/asgn_expr.sv
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
module top;
integer x, y, z;
task check;
input integer a, b, c;
assert (x == a);
assert (y == b);
assert (z == c);
endtask
always_comb begin
x = 0; y = 0; z = 0;
check(0, 0, 0);

// post-increment/decrement statements
x++;
check(1, 0, 0);
y (* foo *) ++;
check(1, 1, 0);
z--;
check(1, 1, -1);
z (* foo *) --;
check(1, 1, -2);

// pre-increment/decrement statements are equivalent
++z;
check(1, 1, -1);
++ (* foo *) z;
check(1, 1, 0);
--x;
check(0, 1, 0);
-- (* foo *) y;
check(0, 0, 0);

// procedural pre-increment/decrement expressions
z = ++x;
check(1, 0, 1);
z = ++ (* foo *) x;
check(2, 0, 2);
y = --x;
check(1, 1, 2);
y = -- (* foo *) x;

// procedural post-increment/decrement expressions
// TODO: support attributes on post-increment/decrement
check(0, 0, 2);
y = x++;
check(1, 0, 2);
y = x--;
check(0, 1, 2);

// procedural assignment expressions
x = (y = (z = 99) + 1) + 1;
check(101, 100, 99);
x = (y *= 2);
check(200, 200, 99);
x = (z >>= 2) * 4;
check(96, 200, 24);
y = (z >>= 1'sb1) * 2; // shift is implicitly cast to unsigned
check(96, 24, 12);
end
endmodule
3 changes: 3 additions & 0 deletions tests/verilog/asgn_expr.ys
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
read_verilog -sv asgn_expr.sv
proc
sat -verify -prove-asserts -show-all
7 changes: 7 additions & 0 deletions tests/verilog/asgn_expr_not_proc_1.ys
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
logger -expect error "Assignments within expressions are only permitted within procedures." 1
read_verilog -sv <<EOF
module top;
integer x, y;
assign x = y++;
endmodule
EOF
7 changes: 7 additions & 0 deletions tests/verilog/asgn_expr_not_proc_2.ys
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
logger -expect error "Assignments within expressions are only permitted within procedures." 1
read_verilog -sv <<EOF
module top;
integer x;
wire [++x:0] y;
endmodule
EOF
7 changes: 7 additions & 0 deletions tests/verilog/asgn_expr_not_proc_3.ys
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
logger -expect error "Assignments within expressions are only permitted within procedures." 1
read_verilog -sv <<EOF
module top;
integer x;
integer y = --x;
endmodule
EOF
7 changes: 7 additions & 0 deletions tests/verilog/asgn_expr_not_proc_4.ys
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
logger -expect error "Assignments within expressions are only permitted within procedures." 1
read_verilog -sv <<EOF
module top;
integer x, y;
assign x = (y = 1);
endmodule
EOF
7 changes: 7 additions & 0 deletions tests/verilog/asgn_expr_not_proc_5.ys
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
logger -expect error "Assignments within expressions are only permitted within procedures." 1
read_verilog -sv <<EOF
module top;
integer x, y;
assign x = (y += 2);
endmodule
EOF
7 changes: 7 additions & 0 deletions tests/verilog/asgn_expr_not_sv_1.ys
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
logger -expect error "Assignments within expressions are only supported in SystemVerilog mode." 1
read_verilog <<EOF
module top;
integer x, y;
initial y = ++x;
endmodule
EOF
7 changes: 7 additions & 0 deletions tests/verilog/asgn_expr_not_sv_2.ys
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
logger -expect error "Assignments within expressions are only supported in SystemVerilog mode." 1
read_verilog <<EOF
module top;
integer x, y;
initial y = x++;
endmodule
EOF
7 changes: 7 additions & 0 deletions tests/verilog/asgn_expr_not_sv_3.ys
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
logger -expect error "Assignments within expressions are only supported in SystemVerilog mode." 1
read_verilog <<EOF
module top;
integer x, y;
initial y = (x = 1);
endmodule
EOF
15 changes: 15 additions & 0 deletions tests/verilog/asgn_expr_not_sv_4.ys
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
read_verilog -sv <<EOF
module top;
integer x, y;
initial y = (x += 1);
endmodule
EOF
design -reset

logger -expect error "syntax error, unexpected TOK_ID" 1
read_verilog <<EOF
module top;
integer x, y;
initial y = (x += 1);
endmodule
EOF
2 changes: 1 addition & 1 deletion tests/verilog/task_attr.ys
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ EOT
select -assert-none a:* a:src %d


logger -expect error "syntax error, unexpected ATTR_BEGIN" 1
logger -expect error "syntax error, unexpected ';', expecting ATTR_BEGIN or TOK_INCREMENT or TOK_DECREMENT" 1
design -reset
read_verilog <<EOT
module top;
Expand Down