From 156f08a1c58b2ea11e42f6d39bb953dc0559bd1f Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Sun, 3 Apr 2022 12:43:31 +0200 Subject: [PATCH] Set default signedness of return types for built-in methods of SV types The result for the built-in methods for the SystemVerilog types is currently always unsigned. This can lead to incorrect behavior if the value is sign extended or passed as an argument to a system function (e.g. $display). For most built-in methods this does not matter, since even though they have a signed return type, they will not return a negative value. E.g. the string `len()` or queue `size()` functions. It does make a difference though for the queue `pop_front()` and `pop_back()` methods. Their return type is the element type of the queue. If the element type is signed and the value in queue is negative is will be handled incorrectly. E.g. the following will print `4294967295` rather than `-1`. ``` int q[$]; q.push_back(-1); $display(q.pop_front()); ``` To correctly support this consistently assign the actual data type of the built-in method's return value to the `NetESFunc`, rather than just the width and base type. The width, base type and also the signedness can be derived from the data type. Note that this only fixes the default signedness, but not the case where the signedness of the expression is changed by its context (e.g. in arithmetic expression). Handling this will require some additional work. Also note that assigning the actual data type is also required to support type checking on the return value, e.g. as needed for enum types. Signed-off-by: Lars-Peter Clausen --- Statement.h | 4 +--- elab_expr.cc | 32 +++++++++++++++++--------------- elaborate.cc | 28 +++++++++++----------------- net_expr.cc | 23 ++++------------------- netlist.h | 1 - 5 files changed, 33 insertions(+), 55 deletions(-) diff --git a/Statement.h b/Statement.h index 80558440a..9ad531f24 100644 --- a/Statement.h +++ b/Statement.h @@ -249,9 +249,7 @@ class PCallTask : public Statement { const char*sys_task_name) const; NetProc*elaborate_method_func_(NetScope*scope, NetNet*net, - ivl_variable_type_t type, - unsigned width, - bool signed_flag, + ivl_type_t type, perm_string method_name, const char*sys_task_name) const; bool test_task_calls_ok_(Design*des, NetScope*scope) const; diff --git a/elab_expr.cc b/elab_expr.cc index 9f993f0f9..939907b02 100644 --- a/elab_expr.cc +++ b/elab_expr.cc @@ -3058,7 +3058,7 @@ NetExpr* PECallFunction::elaborate_expr_method_(Design*des, NetScope*scope, << "takes no arguments" << endl; des->errors += 1; } - NetESFunc*sys_expr = new NetESFunc("$size", IVL_VT_BOOL, 32, 1); + NetESFunc*sys_expr = new NetESFunc("$size", &netvector_t::atom2u32, 1); sys_expr->set_line(*this); sys_expr->parm(0, sub_expr); return sys_expr; @@ -3076,19 +3076,20 @@ NetExpr* PECallFunction::elaborate_expr_method_(Design*des, NetScope*scope, // Get the method name that we are looking for. perm_string method_name = search_results.path_tail.back().name; - if (method_name == "size") { if (parms_.size() != 0) { cerr << get_fileline() << ": error: size() method " << "takes no arguments" << endl; des->errors += 1; } - NetESFunc*sys_expr = new NetESFunc("$size", IVL_VT_BOOL, 32, 1); + NetESFunc*sys_expr = new NetESFunc("$size", &netvector_t::atom2u32, 1); sys_expr->set_line(*this); sys_expr->parm(0, sub_expr); return sys_expr; } + const netqueue_t*queue = search_results.net->queue_type(); + ivl_type_t element_type = queue->element_type(); if (method_name == "pop_back") { if (parms_.size() != 0) { cerr << get_fileline() << ": error: pop_back() method " @@ -3096,7 +3097,7 @@ NetExpr* PECallFunction::elaborate_expr_method_(Design*des, NetScope*scope, des->errors += 1; } NetESFunc*sys_expr = new NetESFunc("$ivl_queue_method$pop_back", - expr_type_, expr_width_, 1); + element_type, 1); sys_expr->set_line(*this); sys_expr->parm(0, sub_expr); return sys_expr; @@ -3109,7 +3110,7 @@ NetExpr* PECallFunction::elaborate_expr_method_(Design*des, NetScope*scope, des->errors += 1; } NetESFunc*sys_expr = new NetESFunc("$ivl_queue_method$pop_front", - expr_type_, expr_width_, 1); + element_type, 1); sys_expr->set_line(*this); sys_expr->parm(0, sub_expr); return sys_expr; @@ -3186,35 +3187,35 @@ NetExpr* PECallFunction::elaborate_expr_method_(Design*des, NetScope*scope, if (method_name == "len") { NetESFunc*sys_expr = new NetESFunc("$ivl_string_method$len", - IVL_VT_BOOL, 32, 1); + &netvector_t::atom2u32, 1); sys_expr->parm(0, sub_expr); return sys_expr; } if (method_name == "atoi") { NetESFunc*sys_expr = new NetESFunc("$ivl_string_method$atoi", - IVL_VT_BOOL, integer_width, 1); + netvector_t::integer_type(), 1); sys_expr->parm(0, sub_expr); return sys_expr; } if (method_name == "atoreal") { NetESFunc*sys_expr = new NetESFunc("$ivl_string_method$atoreal", - IVL_VT_REAL, 1, 1); + &netreal_t::type_real, 1); sys_expr->parm(0, sub_expr); return sys_expr; } if (method_name == "atohex") { NetESFunc*sys_expr = new NetESFunc("$ivl_string_method$atohex", - IVL_VT_BOOL, integer_width, 1); + netvector_t::integer_type(), 1); sys_expr->parm(0, sub_expr); return sys_expr; } if (method_name == "substr") { NetESFunc*sys_expr = new NetESFunc("$ivl_string_method$substr", - IVL_VT_STRING, 1, 3); + &netstring_t::type_string, 3); sys_expr->set_line(*this); // First argument is the source string. @@ -4657,7 +4658,9 @@ NetExpr* PEIdent::elaborate_expr(Design*des, NetScope*scope, ivl_assert(*this, sr.path_tail.size() == 1); const name_component_t member_comp = sr.path_tail.front(); if (member_comp.name == "size") { - NetESFunc*fun = new NetESFunc("$size", IVL_VT_BOOL, 32, 1); + NetESFunc*fun = new NetESFunc("$size", + &netvector_t::atom2s32, + 1); fun->set_line(*this); NetESignal*arg = new NetESignal(sr.net); @@ -4773,11 +4776,10 @@ NetExpr* PEIdent::elaborate_expr(Design*des, NetScope*scope, ivl_assert(*this, sr.path_tail.size() == 1); const name_component_t member_comp = sr.path_tail.front(); const netqueue_t*queue = sr.net->queue_type(); - ivl_variable_type_t qelem_type = queue->element_base_type(); - unsigned qelem_width = queue->element_width(); + ivl_type_t element_type = queue->element_type(); if (member_comp.name == "pop_back") { NetESFunc*fun = new NetESFunc("$ivl_queue_method$pop_back", - qelem_type, qelem_width, 1); + element_type, 1); fun->set_line(*this); NetESignal*arg = new NetESignal(sr.net); @@ -4789,7 +4791,7 @@ NetExpr* PEIdent::elaborate_expr(Design*des, NetScope*scope, if (member_comp.name == "pop_front") { NetESFunc*fun = new NetESFunc("$ivl_queue_method$pop_front", - qelem_type, qelem_width, 1); + element_type, 1); fun->set_line(*this); NetESignal*arg = new NetESignal(sr.net); diff --git a/elaborate.cc b/elaborate.cc index fa799a22e..a5edabd63 100644 --- a/elaborate.cc +++ b/elaborate.cc @@ -3676,9 +3676,7 @@ NetProc* PCallTask::elaborate_queue_method_(Design*des, NetScope*scope, */ NetProc* PCallTask::elaborate_method_func_(NetScope*scope, NetNet*net, - ivl_variable_type_t type, - unsigned width, - bool signed_flag, + ivl_type_t type, perm_string method_name, const char*sys_task_name) const { @@ -3686,15 +3684,14 @@ NetProc* PCallTask::elaborate_method_func_(NetScope*scope, << method_name << "' is being called as a task." << endl; // Generate the function. - NetESFunc*sys_expr = new NetESFunc(sys_task_name, type, width, 1); + NetESFunc*sys_expr = new NetESFunc(sys_task_name, type, 1); sys_expr->set_line(*this); NetESignal*arg = new NetESignal(net); arg->set_line(*net); sys_expr->parm(0, arg); // Create a L-value that matches the function return type. NetNet*tmp; - netvector_t*tmp_vec = new netvector_t(type, width-1, 0, signed_flag); - tmp = new NetNet(scope, scope->local_symbol(), NetNet::REG, tmp_vec); + tmp = new NetNet(scope, scope->local_symbol(), NetNet::REG, type); tmp->set_line(*this); NetAssign_*lv = new NetAssign_(tmp); // Generate an assign to the fake L-value. @@ -3782,9 +3779,8 @@ NetProc* PCallTask::elaborate_method_(Design*des, NetScope*scope, else if (method_name=="size") // This returns an int. It could be removed, but keep for now. return elaborate_method_func_(scope, net, - IVL_VT_BOOL, 32, - true, method_name, - "$size"); + &netvector_t::atom2s32, + method_name, "$size"); else if (method_name=="reverse") { cerr << get_fileline() << ": sorry: 'reverse()' " "array sorting method is not currently supported." @@ -3825,15 +3821,13 @@ NetProc* PCallTask::elaborate_method_(Design*des, NetScope*scope, "$ivl_queue_method$insert"); else if (method_name == "pop_front") return elaborate_method_func_(scope, net, - use_darray->element_base_type(), - use_darray->element_width(), - false, method_name, + use_darray->element_type(), + method_name, "$ivl_queue_method$pop_front"); else if (method_name == "pop_back") return elaborate_method_func_(scope, net, - use_darray->element_base_type(), - use_darray->element_width(), - false, method_name, + use_darray->element_type(), + method_name, "$ivl_queue_method$pop_back"); } @@ -5324,12 +5318,12 @@ NetProc* PForeach::elaborate(Design*des, NetScope*scope) const idx_exp->set_line(*this); // Make an initialization expression for the index. - NetESFunc*init_expr = new NetESFunc("$low", IVL_VT_BOOL, 32, 1); + NetESFunc*init_expr = new NetESFunc("$low", &netvector_t::atom2s32, 1); init_expr->set_line(*this); init_expr->parm(0, array_exp); // Make a condition expression: idx <= $high(array) - NetESFunc*high_exp = new NetESFunc("$high", IVL_VT_BOOL, 32, 1); + NetESFunc*high_exp = new NetESFunc("$high", &netvector_t::atom2s32, 1); high_exp->set_line(*this); high_exp->parm(0, array_exp); diff --git a/net_expr.cc b/net_expr.cc index 5cd29e013..7370a49bd 100644 --- a/net_expr.cc +++ b/net_expr.cc @@ -487,28 +487,13 @@ NetESFunc::NetESFunc(const char*n, ivl_variable_type_t t, } NetESFunc::NetESFunc(const char*n, ivl_type_t rtype, unsigned np) -: NetExpr(rtype), name_(0), type_(IVL_VT_NO_TYPE), enum_type_(0), parms_(np), is_overridden_(false) +: NetExpr(rtype), name_(0), type_(rtype->base_type()), + enum_type_(dynamic_cast(rtype)), parms_(np), + is_overridden_(false) { name_ = lex_strings.add(n); expr_width(rtype->packed_width()); - // FIXME: For now, assume that all uses of this constructor - // are for the IVL_VT_DARRAY type. Eventually, the type_ - // member will go away. - if (dynamic_cast(rtype)) - type_ = IVL_VT_DARRAY; - else if (dynamic_cast(rtype)) - type_ = IVL_VT_CLASS; - else if (dynamic_cast(rtype)) - type_ = IVL_VT_STRING; - else - ivl_assert(*this, 0); -} - -NetESFunc::NetESFunc(const char*n, const netenum_t*enum_type, unsigned np) -: name_(0), type_(enum_type->base_type()), enum_type_(enum_type), parms_(np), is_overridden_(false) -{ - name_ = lex_strings.add(n); - expr_width(enum_type->packed_width()); + cast_signed_base_(rtype->get_signed()); } NetESFunc::~NetESFunc() diff --git a/netlist.h b/netlist.h index 417c0f9ab..2159d5fcf 100644 --- a/netlist.h +++ b/netlist.h @@ -4641,7 +4641,6 @@ class NetESFunc : public NetExpr { NetESFunc(const char*name, ivl_variable_type_t t, unsigned width, unsigned nprms, bool is_overridden =false); NetESFunc(const char*name, ivl_type_t rtype, unsigned nprms); - NetESFunc(const char*name, const netenum_t*enum_type, unsigned nprms); ~NetESFunc(); const char* name() const;