-
Notifications
You must be signed in to change notification settings - Fork 893
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
{ | ||
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} | ||
|
@@ -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 | ||
|
@@ -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!"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -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: | ||
|
@@ -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); | ||
|
@@ -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: | ||
|
@@ -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: | ||
|
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.