From 8de2e60d26150bae29a30c0275503583704a5d15 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Mon, 21 Mar 2022 14:20:35 +0100 Subject: [PATCH 1/7] Consolidate different NetNet constructors There are a couple of different NetNet constructors for different data types. They are all very similar, consolidate them into a single constructor taking a ivl_type_t. Signed-off-by: Lars-Peter Clausen --- netlist.cc | 39 +++++---------------------------------- netlist.h | 7 +------ 2 files changed, 6 insertions(+), 40 deletions(-) diff --git a/netlist.cc b/netlist.cc index cb2ee73a6..aa767c444 100644 --- a/netlist.cc +++ b/netlist.cc @@ -549,6 +549,9 @@ template static unsigned calculate_count(T*type) void NetNet::calculate_slice_widths_from_packed_dims_(void) { ivl_assert(*this, net_type_); + if (!net_type_->packed()) + return; + slice_dims_ = net_type_->slice_dimensions(); // Special case: There are no actual packed dimensions, so @@ -598,42 +601,10 @@ NetNet::NetNet(NetScope*s, perm_string n, Type t, s->add_signal(this); } -/* - * When we create a netnet for a packed struct, create a single - * vector with the msb_/lsb_ chosen to name enough bits for the entire - * packed structure. - */ -NetNet::NetNet(NetScope*s, perm_string n, Type t, netstruct_t*ty) +NetNet::NetNet(NetScope*s, perm_string n, Type t, ivl_type_t type) : NetObj(s, n, 1), type_(t), port_type_(NOT_A_PORT), - local_flag_(false), net_type_(ty), - discipline_(0), - eref_count_(0), lref_count_(0) -{ - //XXXX packed_dims_.push_back(netrange_t(calculate_count(ty)-1, 0)); - calculate_slice_widths_from_packed_dims_(); - - initialize_dir_(); - - s->add_signal(this); -} - -NetNet::NetNet(NetScope*s, perm_string n, Type t, netdarray_t*ty) -: NetObj(s, n, 1), - type_(t), port_type_(NOT_A_PORT), - local_flag_(false), net_type_(ty), - discipline_(0), - eref_count_(0), lref_count_(0) -{ - initialize_dir_(); - - s->add_signal(this); -} - -NetNet::NetNet(NetScope*s, perm_string n, Type t, netvector_t*ty) -: NetObj(s, n, 1), - type_(t), port_type_(NOT_A_PORT), - local_flag_(false), net_type_(ty), + local_flag_(false), net_type_(type), discipline_(0), eref_count_(0), lref_count_(0) { diff --git a/netlist.h b/netlist.h index fdbe730ca..417c0f9ab 100644 --- a/netlist.h +++ b/netlist.h @@ -679,12 +679,7 @@ class NetNet : public NetObj, public PortType { const std::list&unpacked, ivl_type_t type); - // This form builds a NetNet from its record/enum/darray - // definition. They should probably be replaced with a single - // version that takes an ivl_type_s* base. - explicit NetNet(NetScope*s, perm_string n, Type t, netstruct_t*type); - explicit NetNet(NetScope*s, perm_string n, Type t, netdarray_t*type); - explicit NetNet(NetScope*s, perm_string n, Type t, netvector_t*type); + explicit NetNet(NetScope*s, perm_string n, Type t, ivl_type_t type); virtual ~NetNet(); From 156f08a1c58b2ea11e42f6d39bb953dc0559bd1f Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Sun, 3 Apr 2022 12:43:31 +0200 Subject: [PATCH 2/7] 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; From 2f38adaeff8407ab8c540f9533fb9c8f6b91f46e Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Fri, 8 Apr 2022 10:46:16 +0200 Subject: [PATCH 3/7] Report correct queue method return type when call without parenthesis When calling a queue method without parenthesis it gets elaborated through the PEIdent path. On this path the type, width and signdess are not reported for the method call. This leads to incorrect behavior in contexts where those are important. Correctly report the type, the same as when the method is called with parenthesis. Signed-off-by: Lars-Peter Clausen --- elab_expr.cc | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/elab_expr.cc b/elab_expr.cc index 939907b02..7bdaf06aa 100644 --- a/elab_expr.cc +++ b/elab_expr.cc @@ -3965,6 +3965,16 @@ unsigned PEIdent::test_width_method_(Design*des, NetScope*scope, width_mode_t&) } } + if (const struct netqueue_t *queue = net->queue_type()) { + if (member_name == "pop_back" || member_name == "pop_front") { + expr_type_ = queue->element_base_type(); + expr_width_ = queue->element_width(); + min_width_ = expr_width_; + signed_flag_ = queue->get_signed(); + return expr_width_; + } + } + // Look for the enumeration attributes. if (const netenum_t*netenum = net->enumeration()) { if (member_name == "num") { From 22cba1073ccd2a63a514879619fb9419bca3cf24 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Mon, 11 Apr 2022 10:38:00 +0200 Subject: [PATCH 4/7] Handle context signedness and width expansion for method return types The signedness of an expression can change depending on its context. E.g. for an arithmetic operation with one unsigned operand all operands are treated as unsigned. For methods, both on built-in SystemVerilog types as well as user defined classes, this is currently not considered. This can lead to incorrect behavior if the value is sign extended. E.g. the following will print 4294967295 rather than 65535. ``` shortint q[$]; q.push_back(-1); $display(q.pop_front() + 32'h0); ``` Furthermore the return value is not expanded to the width of its context. This can cause vvp to crash with an exception when it expects a vector on the stack to have a certain width. E.g. ``` int d[]; longint x; bit a = 1'b1; x = a ? d.size() : 64'h0; ``` Solve both of this by using `pad_to_width()` on the method return value if it is a vectorable type. Since any function call, not just methods, needs to have this done to its return value insert this on the common path. Signed-off-by: Lars-Peter Clausen --- PExpr.h | 19 +++++++----- elab_expr.cc | 88 ++++++++++++++++++++++++++++------------------------ net_expr.cc | 1 + netlist.cc | 1 + 4 files changed, 60 insertions(+), 49 deletions(-) diff --git a/PExpr.h b/PExpr.h index 6ee7001f5..c02630d63 100644 --- a/PExpr.h +++ b/PExpr.h @@ -926,22 +926,25 @@ class PECallFunction : public PExpr { NetExpr* cast_to_width_(NetExpr*expr, unsigned wid) const; + NetExpr* elaborate_expr_(Design *des, NetScope *scope, + unsigned flags) const; + NetExpr*elaborate_expr_pkg_(Design*des, NetScope*scope, - unsigned expr_wid, unsigned flags)const; + unsigned flags)const; NetExpr* elaborate_expr_method_(Design*des, NetScope*scope, - symbol_search_results&search_results, - unsigned expr_wid) const; + symbol_search_results&search_results) + const; NetExpr* elaborate_expr_method_par_(Design*des, NetScope*scope, - symbol_search_results&search_results, - unsigned expr_wid) const; + symbol_search_results&search_results) + const; NetExpr* elaborate_sfunc_(Design*des, NetScope*scope, unsigned expr_wid, unsigned flags) const; - NetExpr* elaborate_access_func_(Design*des, NetScope*scope, ivl_nature_t, - unsigned expr_wid) const; + NetExpr* elaborate_access_func_(Design*des, NetScope*scope, ivl_nature_t) + const; unsigned test_width_sfunc_(Design*des, NetScope*scope, width_mode_t&mode); unsigned test_width_method_(Design*des, NetScope*scope, @@ -949,7 +952,7 @@ class PECallFunction : public PExpr { width_mode_t&mode); NetExpr*elaborate_base_(Design*des, NetScope*scope, NetScope*dscope, - unsigned expr_wid, unsigned flags) const; + unsigned flags) const; unsigned elaborate_arguments_(Design*des, NetScope*scope, NetFuncDef*def, bool need_const, diff --git a/elab_expr.cc b/elab_expr.cc index 7bdaf06aa..e980e2188 100644 --- a/elab_expr.cc +++ b/elab_expr.cc @@ -1873,8 +1873,7 @@ NetExpr* PECallFunction::elaborate_sfunc_(Design*des, NetScope*scope, } NetExpr* PECallFunction::elaborate_access_func_(Design*des, NetScope*scope, - ivl_nature_t nature, - unsigned expr_wid) const + ivl_nature_t nature) const { // An access function must have 1 or 2 arguments. ivl_assert(*this, parms_.size()==2 || parms_.size()==1); @@ -1925,8 +1924,7 @@ NetExpr* PECallFunction::elaborate_access_func_(Design*des, NetScope*scope, NetExpr*tmp = new NetEAccess(branch, nature); tmp->set_line(*this); - - return pad_to_width(tmp, expr_wid, signed_flag_, *this); + return tmp; } /* @@ -1938,7 +1936,6 @@ static NetExpr* check_for_enum_methods(const LineInfo*li, const pform_name_t&use_path, perm_string method_name, NetExpr*expr, - unsigned rtn_wid, PExpr*parg, unsigned args) { if (debug_elaborate) { @@ -1948,8 +1945,6 @@ static NetExpr* check_for_enum_methods(const LineInfo*li, << endl; cerr << li->get_fileline() << ": " << __func__ << ": " << "use_path=" << use_path << endl; - cerr << li->get_fileline() << ": " << __func__ << ": " - << "rtn_wid=" << rtn_wid << endl; cerr << li->get_fileline() << ": " << __func__ << ": " << "expr=" << *expr << endl; } @@ -2569,7 +2564,6 @@ NetExpr* PEIdent::elaborate_expr_class_field_(Design*des, NetScope*scope, } NetExpr* PECallFunction::elaborate_expr_pkg_(Design*des, NetScope*scope, - unsigned expr_wid, unsigned flags) const { if (debug_elaborate) { @@ -2594,7 +2588,7 @@ NetExpr* PECallFunction::elaborate_expr_pkg_(Design*des, NetScope*scope, if (! check_call_matches_definition_(des, dscope)) return 0; - return elaborate_base_(des, scope, dscope, expr_wid, flags); + return elaborate_base_(des, scope, dscope, flags); } NetExpr* PECallFunction::elaborate_expr(Design*des, NetScope*scope, @@ -2611,14 +2605,24 @@ NetExpr* PECallFunction::elaborate_expr(Design*des, NetScope*scope, << " at " << package_->get_fileline() << endl; } - if (package_) - return elaborate_expr_pkg_(des, scope, expr_wid, flags); - - flags &= ~SYS_TASK_ARG; // don't propagate the SYS_TASK_ARG flag - if (peek_tail_name(path_)[0] == '$') return elaborate_sfunc_(des, scope, expr_wid, flags); + NetExpr *result = elaborate_expr_(des, scope, flags); + if (!result || !type_is_vectorable(expr_type_)) + return result; + + return pad_to_width(result, expr_wid, signed_flag_, *this); +} + +NetExpr* PECallFunction::elaborate_expr_(Design*des, NetScope*scope, + unsigned flags) const +{ + if (package_) + return elaborate_expr_pkg_(des, scope, flags); + + flags &= ~SYS_TASK_ARG; // don't propagate the SYS_TASK_ARG flag + // Search for the symbol. This should turn up a scope. symbol_search_results search_results; bool search_flag = symbol_search(this, des, scope, path_, &search_results); @@ -2655,7 +2659,7 @@ NetExpr* PECallFunction::elaborate_expr(Design*des, NetScope*scope, // Maybe this is a method of an object? Give it a try. if (!search_results.path_tail.empty()) { - NetExpr*tmp = elaborate_expr_method_(des, scope, search_results, expr_wid); + NetExpr*tmp = elaborate_expr_method_(des, scope, search_results); if (tmp) { if (debug_elaborate) { cerr << get_fileline() << ": PECallFunction::elaborate_expr: " @@ -2688,8 +2692,7 @@ NetExpr* PECallFunction::elaborate_expr(Design*des, NetScope*scope, // way. ivl_nature_t access_nature = find_access_function(path_); if (access_nature) - return elaborate_access_func_(des, scope, access_nature, - expr_wid); + return elaborate_access_func_(des, scope, access_nature); // Nothing was found so report this as an error. cerr << get_fileline() << ": error: No function named `" << path_ @@ -2726,8 +2729,7 @@ NetExpr* PECallFunction::elaborate_expr(Design*des, NetScope*scope, use_search_results.net = scope->find_signal(perm_string::literal(THIS_TOKEN)); ivl_assert(*this, use_search_results.net); - NetExpr*tmp = elaborate_expr_method_(des, scope, use_search_results, expr_wid); - return tmp; + return elaborate_expr_method_(des, scope, use_search_results); } } @@ -2753,7 +2755,7 @@ NetExpr* PECallFunction::elaborate_expr(Design*des, NetScope*scope, scope->is_const_func(false); } - return elaborate_base_(des, scope, dscope, expr_wid, flags); + return elaborate_base_(des, scope, dscope, flags); } NetExpr* PECallFunction::elaborate_expr(Design*des, NetScope*scope, @@ -2766,7 +2768,7 @@ NetExpr* PECallFunction::elaborate_expr(Design*des, NetScope*scope, } NetExpr* PECallFunction::elaborate_base_(Design*des, NetScope*scope, NetScope*dscope, - unsigned expr_wid, unsigned flags) const + unsigned flags) const { if (! check_call_matches_definition_(des, dscope)) @@ -2845,11 +2847,7 @@ NetExpr* PECallFunction::elaborate_base_(Design*des, NetScope*scope, NetScope*ds NetESignal*eres = new NetESignal(res); NetEUFunc*func = new NetEUFunc(scope, dscope, eres, parms, need_const); func->set_line(*this); - - if(res->darray_type()) - return func; - - return pad_to_width(func, expr_wid, signed_flag_, *this); + return func; } cerr << get_fileline() << ": internal error: Unable to locate " @@ -2960,8 +2958,8 @@ unsigned PECallFunction::elaborate_arguments_(Design*des, NetScope*scope, * "len" in path_tail, and if x is a string object, we can handle the case. */ NetExpr* PECallFunction::elaborate_expr_method_(Design*des, NetScope*scope, - symbol_search_results&search_results, - unsigned expr_wid) const + symbol_search_results&search_results) + const { if (!gn_system_verilog()) { cerr << get_fileline() << ": error: " @@ -3000,7 +2998,7 @@ NetExpr* PECallFunction::elaborate_expr_method_(Design*des, NetScope*scope, } if (search_results.par_val && search_results.par_type) { - return elaborate_expr_method_par_(des, scope, search_results, expr_wid); + return elaborate_expr_method_par_(des, scope, search_results); } NetExpr* sub_expr = 0; @@ -3135,8 +3133,7 @@ NetExpr* PECallFunction::elaborate_expr_method_(Design*des, NetScope*scope, return check_for_enum_methods(this, des, scope, netenum, path_, method_name, sub_expr, - expr_wid, tmp, - parms_.size()); + tmp, parms_.size()); } // Class methods. Generate function call to the class method. @@ -3250,8 +3247,8 @@ NetExpr* PECallFunction::elaborate_expr_method_(Design*des, NetScope*scope, * stable in the sense that they generate a constant value for a constant input. */ NetExpr* PECallFunction::elaborate_expr_method_par_(Design*des, NetScope*scope, - symbol_search_results&search_results, - unsigned expr_wid) const + symbol_search_results&search_results) + const { ivl_assert(*this, search_results.par_val); ivl_assert(*this, search_results.par_type); @@ -3273,13 +3270,13 @@ NetExpr* PECallFunction::elaborate_expr_method_par_(Design*des, NetScope*scope, if (method_name=="len") { NetEConst*use_val = make_const_val(par_value.size()); use_val->set_line(*this); - return pad_to_width(use_val, expr_wid, *this); + return use_val; } if (method_name == "atoi") { NetEConst*use_val = make_const_val(atoi(par_value.c_str())); use_val->set_line(*this); - return pad_to_width(use_val, expr_wid, true, *this); + return use_val; } if (method_name == "atoreal") { @@ -3291,7 +3288,7 @@ NetExpr* PECallFunction::elaborate_expr_method_par_(Design*des, NetScope*scope, if (method_name == "atohex") { NetEConst*use_val = make_const_val(strtoul(par_value.c_str(),0,16)); use_val->set_line(*this); - return pad_to_width(use_val, expr_wid, true, *this); + return use_val; } // Returning 0 here will cause the caller to print an error @@ -4677,7 +4674,9 @@ NetExpr* PEIdent::elaborate_expr(Design*des, NetScope*scope, arg->set_line(*sr.net); fun->parm(0, arg); - return fun; + if (!fun) + return 0; + return pad_to_width(fun, expr_wid, signed_flag_, *this); } else if (member_comp.name == "find") { cerr << get_fileline() << ": sorry: 'find()' " "array location method is not currently " @@ -4796,7 +4795,9 @@ NetExpr* PEIdent::elaborate_expr(Design*des, NetScope*scope, arg->set_line(*sr.net); fun->parm(0, arg); - return fun; + if (!fun || !type_is_vectorable(expr_type_)) + return fun; + return pad_to_width(fun, expr_wid, signed_flag_, *this); } if (member_comp.name == "pop_front") { @@ -4808,7 +4809,9 @@ NetExpr* PEIdent::elaborate_expr(Design*des, NetScope*scope, arg->set_line(*sr.net); fun->parm(0, arg); - return fun; + if (!fun || !type_is_vectorable(expr_type_)) + return fun; + return pad_to_width(fun, expr_wid, signed_flag_, *this); } } @@ -4858,10 +4861,13 @@ 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(); ivl_assert(*this, member_comp.index.empty()); - return check_for_enum_methods(this, des, use_scope, + NetExpr *tmp = check_for_enum_methods(this, des, use_scope, netenum, sr.path_head, member_comp.name, - expr, expr_wid, NULL, 0); + expr, NULL, 0); + if (!tmp) + return 0; + return pad_to_width(tmp, expr_wid, signed_flag_, *this); } ivl_assert(*this, sr.path_tail.empty()); diff --git a/net_expr.cc b/net_expr.cc index 7370a49bd..bffe3d05e 100644 --- a/net_expr.cc +++ b/net_expr.cc @@ -561,6 +561,7 @@ ivl_variable_type_t NetEShallowCopy::expr_type() const NetEAccess::NetEAccess(NetBranch*br, ivl_nature_t nat) : branch_(br), nature_(nat) { + cast_signed_base_(true); } NetEAccess::~NetEAccess() diff --git a/netlist.cc b/netlist.cc index aa767c444..bd9ef6168 100644 --- a/netlist.cc +++ b/netlist.cc @@ -2159,6 +2159,7 @@ NetEUFunc::NetEUFunc(NetScope*scope, NetScope*def, NetESignal*res, : scope_(scope), func_(def), result_sig_(res), parms_(p), need_const_(nc) { expr_width(result_sig_->expr_width()); + cast_signed_base_(result_sig_->has_sign()); } NetEUFunc::~NetEUFunc() From 3a26bbb59a0d5edfd6eba8338807fc951f0f2318 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Thu, 7 Apr 2022 11:35:05 +0200 Subject: [PATCH 5/7] Handle context signedness and width expansion for class properties The signedness of an expression can change depending on its context. E.g. for an arithmetic operation with one unsigned operand all operands are treated as unsigned. This is currently not considered when accessing class properties. This can lead to incorrect behavior with regards to sign extension. E.g. the following will print 4294967295 rather than 65535. ``` class C; shortint x = -1; endclass ... C c = new; $display(c.x + 32'h0); ``` Furthermore the return value is not expanded to the width of its context. This can cause vvp to crash with an exception when it expects a vector on the stack to have a certain width. E.g. ``` class C; shortint x = -1; endclass ... C c = new; int x; bit a = 1'b1; x = a ? c.x : 64'h0; ``` Solve both of this by using `pad_to_width()` on the property expression if it is a vectorable type. Since any identifier, not just class properties, need to have this done insert this on the common path and remove the `pad_to_width()` call on individual paths. Signed-off-by: Lars-Peter Clausen --- PExpr.h | 3 +++ elab_expr.cc | 51 +++++++++++++++++++++++---------------------------- 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/PExpr.h b/PExpr.h index c02630d63..a57a69bb4 100644 --- a/PExpr.h +++ b/PExpr.h @@ -430,6 +430,9 @@ class PEIdent : public PExpr { NetAssign_*) const; private: + NetExpr* elaborate_expr_(Design *des, NetScope *scope, + unsigned expr_wid, unsigned flags) const; + NetExpr*elaborate_expr_param_or_specparam_(Design*des, NetScope*scope, const NetExpr*par, diff --git a/elab_expr.cc b/elab_expr.cc index e980e2188..35e117f11 100644 --- a/elab_expr.cc +++ b/elab_expr.cc @@ -4539,6 +4539,18 @@ NetExpr* PEIdent::elaborate_expr_class_member_(Design*des, NetScope*scope, */ NetExpr* PEIdent::elaborate_expr(Design*des, NetScope*scope, unsigned expr_wid, unsigned flags) const +{ + NetExpr *result; + + result = elaborate_expr_(des, scope, expr_wid, flags); + if (!result || !type_is_vectorable(expr_type_)) + return result; + + return pad_to_width(result, expr_wid, signed_flag_, *this); +} + +NetExpr* PEIdent::elaborate_expr_(Design*des, NetScope*scope, + unsigned expr_wid, unsigned flags) const { ivl_assert(*this, scope); @@ -4604,13 +4616,9 @@ NetExpr* PEIdent::elaborate_expr(Design*des, NetScope*scope, des->errors += 1; } - NetExpr*tmp = elaborate_expr_param_or_specparam_(des, scope, sr.par_val, - sr.scope, sr.par_type, - expr_wid, flags); - - if (!tmp) return 0; - - return pad_to_width(tmp, expr_wid, signed_flag_, *this); + return elaborate_expr_param_or_specparam_(des, scope, sr.par_val, + sr.scope, sr.par_type, + expr_wid, flags); } // If the identifier names a signal (a variable or a net) @@ -4645,11 +4653,9 @@ NetExpr* PEIdent::elaborate_expr(Design*des, NetScope*scope, << endl; } - NetExpr*tmp = check_for_struct_members(this, des, use_scope, - sr.net, sr.path_head.back().index, - sr.path_tail); - if (!tmp) return 0; - else return pad_to_width(tmp, expr_wid, signed_flag_, *this); + return check_for_struct_members(this, des, use_scope, sr.net, + sr.path_head.back().index, + sr.path_tail); } // If this is an array object, and there are members in @@ -4674,9 +4680,7 @@ NetExpr* PEIdent::elaborate_expr(Design*des, NetScope*scope, arg->set_line(*sr.net); fun->parm(0, arg); - if (!fun) - return 0; - return pad_to_width(fun, expr_wid, signed_flag_, *this); + return fun; } else if (member_comp.name == "find") { cerr << get_fileline() << ": sorry: 'find()' " "array location method is not currently " @@ -4795,9 +4799,7 @@ NetExpr* PEIdent::elaborate_expr(Design*des, NetScope*scope, arg->set_line(*sr.net); fun->parm(0, arg); - if (!fun || !type_is_vectorable(expr_type_)) - return fun; - return pad_to_width(fun, expr_wid, signed_flag_, *this); + return fun; } if (member_comp.name == "pop_front") { @@ -4809,9 +4811,7 @@ NetExpr* PEIdent::elaborate_expr(Design*des, NetScope*scope, arg->set_line(*sr.net); fun->parm(0, arg); - if (!fun || !type_is_vectorable(expr_type_)) - return fun; - return pad_to_width(fun, expr_wid, signed_flag_, *this); + return fun; } } @@ -4861,13 +4861,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(); ivl_assert(*this, member_comp.index.empty()); - NetExpr *tmp = check_for_enum_methods(this, des, use_scope, + return check_for_enum_methods(this, des, use_scope, netenum, sr.path_head, member_comp.name, expr, NULL, 0); - if (!tmp) - return 0; - return pad_to_width(tmp, expr_wid, signed_flag_, *this); } ivl_assert(*this, sr.path_tail.empty()); @@ -4883,7 +4880,7 @@ NetExpr* PEIdent::elaborate_expr(Design*des, NetScope*scope, << ", tmp=" << *tmp << endl; } - return pad_to_width(tmp, expr_wid, signed_flag_, *this); + return tmp; } // If the identifier is a named event @@ -5514,8 +5511,6 @@ NetExpr* PEIdent::elaborate_expr_param_(Design*des, << "Elaborate parameter <" << path_ << "> as enumeration constant." << *etmp << endl; tmp = etmp->dup_expr(); - tmp = pad_to_width(tmp, expr_wid, signed_flag_, *this); - } else { perm_string name = peek_tail_name(path_); From 0c123b8498734d2c9ab46c8c93dc226ca0a2f891 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Sun, 3 Apr 2022 14:02:04 +0200 Subject: [PATCH 6/7] Add regression tests for methods with signed return values Check that the signedness of the return value of methods is handled correctly. * When sign extending * When passing as a value to a system function Check this for both methods on user defined class as well as built-in methods on SystemVerilog types. Signed-off-by: Lars-Peter Clausen --- ivtest/ivltests/enum_method_signed1.v | 156 ++++++++++++++++++++++ ivtest/ivltests/enum_method_signed2.v | 35 +++++ ivtest/ivltests/enum_method_signed3.v | 156 ++++++++++++++++++++++ ivtest/ivltests/enum_method_signed4.v | 35 +++++ ivtest/ivltests/sv_class_method_signed1.v | 71 ++++++++++ ivtest/ivltests/sv_class_method_signed2.v | 30 +++++ ivtest/ivltests/sv_queue_method_signed1.v | 99 ++++++++++++++ ivtest/ivltests/sv_queue_method_signed2.v | 29 ++++ ivtest/ivltests/sv_queue_method_signed3.v | 99 ++++++++++++++ ivtest/ivltests/sv_queue_method_signed4.v | 29 ++++ ivtest/regress-sv.list | 10 ++ ivtest/regress-vlog95.list | 10 ++ 12 files changed, 759 insertions(+) create mode 100644 ivtest/ivltests/enum_method_signed1.v create mode 100644 ivtest/ivltests/enum_method_signed2.v create mode 100644 ivtest/ivltests/enum_method_signed3.v create mode 100644 ivtest/ivltests/enum_method_signed4.v create mode 100644 ivtest/ivltests/sv_class_method_signed1.v create mode 100644 ivtest/ivltests/sv_class_method_signed2.v create mode 100644 ivtest/ivltests/sv_queue_method_signed1.v create mode 100644 ivtest/ivltests/sv_queue_method_signed2.v create mode 100644 ivtest/ivltests/sv_queue_method_signed3.v create mode 100644 ivtest/ivltests/sv_queue_method_signed4.v diff --git a/ivtest/ivltests/enum_method_signed1.v b/ivtest/ivltests/enum_method_signed1.v new file mode 100644 index 000000000..dc5f37cc9 --- /dev/null +++ b/ivtest/ivltests/enum_method_signed1.v @@ -0,0 +1,156 @@ +// Check that the signedness of methods on the built-in enum type is handled +// correctly when calling the method with parenthesis. + +module test; + + bit failed = 1'b0; + + `define check(x) \ + if (!(x)) begin \ + $display("FAILED(%0d): ", `__LINE__, `"x`"); \ + failed = 1'b1; \ + end + + int unsigned x = 10; + int y = 10; + int z; + + enum shortint { + A = -1, + B = -2, + C = -3 + } es; + + enum bit [15:0] { + X = 65535, + Y = 65534, + Z = 65533 + } eu; + + initial begin + es = B; + eu = Y; + + // These all evaluate as signed + `check($signed(eu.first()) < 0) + `check(es.first() < 0) + + `check($signed(eu.last()) < 0) + `check(es.last() < 0) + + `check($signed(eu.prev()) < 0) + `check(es.prev() < 0) + + `check($signed(eu.next()) < 0) + `check(es.next() < 0) + + // These all evaluate as unsigned + `check(eu.first() > 0) + `check({es.first()} > 0) + `check($unsigned(es.first()) > 0) + `check(es.first() > 16'h0) + + `check(eu.last() > 0) + `check({es.last()} > 0) + `check($unsigned(es.last()) > 0) + `check(es.last() > 16'h0) + + `check(eu.prev() > 0) + `check({es.prev()} > 0) + `check($unsigned(es.prev()) > 0) + `check(es.prev() > 16'h0) + + `check(eu.next() > 0) + `check({es.next()} > 0) + `check($unsigned(es.next()) > 0) + `check(es.next() > 16'h0) + + // In arithmetic expressions if one operand is unsigned all operands are + // considered unsigned + z = eu.first() + x; + `check(z === 65545) + z = eu.first() + y; + `check(z === 65545) + + z = eu.last() + x; + `check(z === 65543) + z = eu.last() + y; + `check(z === 65543) + + z = eu.prev() + x; + `check(z === 65545) + z = eu.prev() + y; + `check(z === 65545) + + z = eu.next() + x; + `check(z === 65543) + z = eu.next() + y; + `check(z === 65543) + + z = es.first() + x; + `check(z === 65545) + z = es.first() + y; + `check(z === 9) + + z = es.last() + x; + `check(z === 65543) + z = es.last() + y; + `check(z === 7) + + z = es.prev() + x; + `check(z === 65545) + z = es.prev() + y; + `check(z === 9) + + z = es.next() + x; + `check(z === 65543) + z = es.next() + y; + `check(z === 7) + + // For ternary operators if one operand is unsigned the result is unsigend + z = x ? eu.first() : x; + `check(z === 65535) + z = x ? eu.first() : y; + `check(z === 65535) + + z = x ? eu.last() : x; + `check(z === 65533) + z = x ? eu.last() : y; + `check(z === 65533) + + z = x ? eu.prev() : x; + `check(z === 65535) + z = x ? eu.prev() : y; + `check(z === 65535) + + z = x ? eu.next() : x; + `check(z === 65533) + z = x ? eu.next() : y; + `check(z === 65533) + + z = x ? es.first() : x; + `check(z === 65535) + z = x ? es.first() : y; + `check(z === -1) + + z = x ? es.last() : x; + `check(z === 65533) + z = x ? es.last() : y; + `check(z === -3) + + z = x ? es.prev() : x; + `check(z === 65535) + z = x ? es.prev() : y; + `check(z === -1) + + z = x ? es.next() : x; + `check(z === 65533) + z = x ? es.next() : y; + `check(z === -3) + + if (!failed) begin + $display("PASSED"); + end + end + +endmodule diff --git a/ivtest/ivltests/enum_method_signed2.v b/ivtest/ivltests/enum_method_signed2.v new file mode 100644 index 000000000..9528bebaf --- /dev/null +++ b/ivtest/ivltests/enum_method_signed2.v @@ -0,0 +1,35 @@ +// Check that the signedness of methods on the built-in enum type is handled +// correctly when calling the function with parenthesis and passing the result +// to a system function. + +module test; + + enum shortint { + A = -1, + B = -2, + C = -3 + } es; + + enum bit [15:0] { + X = 65535, + Y = 65534, + Z = 65533 + } eu; + + string s; + + initial begin + es = B; + eu = Y; + + s = $sformatf("%0d %0d %0d %0d %0d %0d %0d %0d", + es.first(), es.last(), es.prev(), es.next(), + eu.first(), eu.last(), eu.prev(), eu.next()); + if (s == "-1 -3 -1 -3 65535 65533 65535 65533") begin + $display("PASSED"); + end else begin + $display("FAILED s=%s", s); + end + end + +endmodule diff --git a/ivtest/ivltests/enum_method_signed3.v b/ivtest/ivltests/enum_method_signed3.v new file mode 100644 index 000000000..ebcbd3e53 --- /dev/null +++ b/ivtest/ivltests/enum_method_signed3.v @@ -0,0 +1,156 @@ +// Check that the signedness of methods on the built-in enum type is handled +// correctly when calling the method without parenthesis. + +module test; + + bit failed = 1'b0; + + `define check(x) \ + if (!(x)) begin \ + $display("FAILED(%0d): ", `__LINE__, `"x`"); \ + failed = 1'b1; \ + end + + int unsigned x = 10; + int y = 10; + int z; + + enum shortint { + A = -1, + B = -2, + C = -3 + } es; + + enum bit [15:0] { + X = 65535, + Y = 65534, + Z = 65533 + } eu; + + initial begin + es = B; + eu = Y; + + // These all evaluate as signed + `check($signed(eu.first) < 0) + `check(es.first < 0) + + `check($signed(eu.last) < 0) + `check(es.last < 0) + + `check($signed(eu.prev) < 0) + `check(es.prev < 0) + + `check($signed(eu.next) < 0) + `check(es.next < 0) + + // These all evaluate as unsigned + `check(eu.first > 0) + `check({es.first} > 0) + `check($unsigned(es.first) > 0) + `check(es.first > 16'h0) + + `check(eu.last > 0) + `check({es.last} > 0) + `check($unsigned(es.last) > 0) + `check(es.last > 16'h0) + + `check(eu.prev > 0) + `check({es.prev} > 0) + `check($unsigned(es.prev) > 0) + `check(es.prev > 16'h0) + + `check(eu.next > 0) + `check({es.next} > 0) + `check($unsigned(es.next) > 0) + `check(es.next > 16'h0) + + // In arithmetic expressions if one operand is unsigned all operands are + // considered unsigned + z = eu.first + x; + `check(z === 65545) + z = eu.first + y; + `check(z === 65545) + + z = eu.last + x; + `check(z === 65543) + z = eu.last + y; + `check(z === 65543) + + z = eu.prev + x; + `check(z === 65545) + z = eu.prev + y; + `check(z === 65545) + + z = eu.next + x; + `check(z === 65543) + z = eu.next + y; + `check(z === 65543) + + z = es.first + x; + `check(z === 65545) + z = es.first + y; + `check(z === 9) + + z = es.last + x; + `check(z === 65543) + z = es.last + y; + `check(z === 7) + + z = es.prev + x; + `check(z === 65545) + z = es.prev + y; + `check(z === 9) + + z = es.next + x; + `check(z === 65543) + z = es.next + y; + `check(z === 7) + + // For ternary operators if one operand is unsigned the result is unsigend + z = x ? eu.first : x; + `check(z === 65535) + z = x ? eu.first : y; + `check(z === 65535) + + z = x ? eu.last : x; + `check(z === 65533) + z = x ? eu.last : y; + `check(z === 65533) + + z = x ? eu.prev : x; + `check(z === 65535) + z = x ? eu.prev : y; + `check(z === 65535) + + z = x ? eu.next : x; + `check(z === 65533) + z = x ? eu.next : y; + `check(z === 65533) + + z = x ? es.first : x; + `check(z === 65535) + z = x ? es.first : y; + `check(z === -1) + + z = x ? es.last : x; + `check(z === 65533) + z = x ? es.last : y; + `check(z === -3) + + z = x ? es.prev : x; + `check(z === 65535) + z = x ? es.prev : y; + `check(z === -1) + + z = x ? es.next : x; + `check(z === 65533) + z = x ? es.next : y; + `check(z === -3) + + if (!failed) begin + $display("PASSED"); + end + end + +endmodule diff --git a/ivtest/ivltests/enum_method_signed4.v b/ivtest/ivltests/enum_method_signed4.v new file mode 100644 index 000000000..4e7308ae0 --- /dev/null +++ b/ivtest/ivltests/enum_method_signed4.v @@ -0,0 +1,35 @@ +// Check that the signedness of methods on the built-in enum type is handled +// correctly when calling the function without parenthesis and passing the +// result to a system function. + +module test; + + enum shortint { + A = -1, + B = -2, + C = -3 + } es; + + enum bit [15:0] { + X = 65535, + Y = 65534, + Z = 65533 + } eu; + + string s; + + initial begin + es = B; + eu = Y; + + s = $sformatf("%0d %0d %0d %0d %0d %0d %0d %0d", + es.first, es.last, es.prev, es.next, + eu.first, eu.last, eu.prev, eu.next); + if (s == "-1 -3 -1 -3 65535 65533 65535 65533") begin + $display("PASSED"); + end else begin + $display("FAILED s=%s", s); + end + end + +endmodule diff --git a/ivtest/ivltests/sv_class_method_signed1.v b/ivtest/ivltests/sv_class_method_signed1.v new file mode 100644 index 000000000..35f7ef633 --- /dev/null +++ b/ivtest/ivltests/sv_class_method_signed1.v @@ -0,0 +1,71 @@ +// Check that the signedness of methods on user defined classes is handled +// correctly. + +module test; + + bit failed = 1'b0; + + `define check(x) \ + if (!(x)) begin \ + $display("FAILED(%0d): ", `__LINE__, `"x`"); \ + failed = 1'b1; \ + end + + class C; + function shortint s; + return -1; + endfunction + + function bit [15:0] u; + return -1; + endfunction + endclass + + C c; + + int unsigned x = 10; + int y = 10; + int z; + + initial begin + c = new; + + // These all evaluate as signed + `check($signed(c.u()) < 0) + `check(c.s() < 0) + + // These all evaluate as unsigned + `check(c.u() > 0) + `check({c.s()} > 0) + `check($unsigned(c.s()) > 0) + `check(c.s() > 16'h0) + + // In arithmetic expressions if one operand is unsigned all operands are + // considered unsigned + z = c.u() + x; + `check(z === 65545) + z = c.u() + y; + `check(z === 65545) + + z = c.s() + x; + `check(z === 65545) + z = c.s() + y; + `check(z === 9) + + // For ternary operators if one operand is unsigned the result is unsigend + z = x ? c.u() : x; + `check(z === 65535) + z = x ? c.u() : y; + `check(z === 65535) + + z = x ? c.s() : x; + `check(z === 65535) + z = x ? c.s() : y; + `check(z === -1) + + if (!failed) begin + $display("PASSED"); + end + end + +endmodule diff --git a/ivtest/ivltests/sv_class_method_signed2.v b/ivtest/ivltests/sv_class_method_signed2.v new file mode 100644 index 000000000..8bf0b0bc5 --- /dev/null +++ b/ivtest/ivltests/sv_class_method_signed2.v @@ -0,0 +1,30 @@ +// Check that the signedness of methods on user defined classes is handled +// correctly when passing the result to a system function. + +module test; + + class C; + function shortint s; + return -1; + endfunction + + function bit [15:0] u; + return -1; + endfunction + endclass + + C c; + string s; + + initial begin + c = new; + + s = $sformatf("%0d %0d", c.s(), c.u()); + if (s == "-1 65535") begin + $display("PASSED"); + end else begin + $display("FAILED s=%s", s); + end + end + +endmodule diff --git a/ivtest/ivltests/sv_queue_method_signed1.v b/ivtest/ivltests/sv_queue_method_signed1.v new file mode 100644 index 000000000..f3f3c7cd2 --- /dev/null +++ b/ivtest/ivltests/sv_queue_method_signed1.v @@ -0,0 +1,99 @@ +// Check that the signedness of the element type of a queue is correctly handled +// whenn calling one of the pop methods with parenthesis. + +module test; + + bit failed = 1'b0; + + `define check(x) \ + if (!(x)) begin \ + $display("FAILED(%0d): ", `__LINE__, `"x`"); \ + failed = 1'b1; \ + end + + int unsigned x = 10; + int y = 10; + int z; + longint w; + + shortint qs[$]; + bit [15:0] qu[$]; + + initial begin + for (int i = 0; i < 16; i++) begin + qu.push_back(-1); + qs.push_back(-1); + end + + // These all evaluate as signed + `check($signed(qu.pop_back()) < 0) + `check(qs.pop_back() < 0) + + `check($signed(qu.pop_front()) < 0) + `check(qs.pop_front() < 0) + + // These all evaluate as unsigned + `check(qu.pop_back() > 0) + `check({qs.pop_back()} > 0) + `check($unsigned(qs.pop_back()) > 0) + `check(qs.pop_back() > 16'h0) + + `check(qu.pop_front() > 0) + `check({qs.pop_front()} > 0) + `check($unsigned(qs.pop_front()) > 0) + `check(qs.pop_front() > 16'h0) + + // In arithmetic expressions if one operand is unsigned all operands are + // considered unsigned + z = qu.pop_back() + x; + `check(z === 65545) + z = qu.pop_back() + y; + `check(z === 65545) + + z = qu.pop_front() + x; + `check(z === 65545) + z = qu.pop_front() + y; + `check(z === 65545) + + z = qs.pop_back() + x; + `check(z === 65545) + z = qs.pop_back() + y; + `check(z === 9) + + z = qs.pop_front() + x; + `check(z === 65545) + z = qs.pop_front() + y; + `check(z === 9) + + // For ternary operators if one operand is unsigned the result is unsigend + z = x ? qu.pop_back() : x; + `check(z === 65535) + z = x ? qu.pop_back() : y; + `check(z === 65535) + + z = x ? qu.pop_front() : x; + `check(z === 65535) + z = x ? qu.pop_front() : y; + `check(z === 65535) + + z = x ? qs.pop_back() : x; + `check(z === 65535) + z = x ? qs.pop_back() : y; + `check(z === -1) + + z = x ? qs.pop_front() : x; + `check(z === 65535) + z = x ? qs.pop_front() : y; + `check(z === -1) + + // Size return value is always positive, but check that it gets padded + // properly + w = x ? qu.size() : 64'h123; + `check(w === 64'h4) + + if (!failed) begin + $display("PASSED"); + end + end + +endmodule diff --git a/ivtest/ivltests/sv_queue_method_signed2.v b/ivtest/ivltests/sv_queue_method_signed2.v new file mode 100644 index 000000000..06831c583 --- /dev/null +++ b/ivtest/ivltests/sv_queue_method_signed2.v @@ -0,0 +1,29 @@ +// Check that the signedness of the element type of a queue is correctly handled +// when passing the result of one of the pop methods as an argument to a system +// function. + +module test; + + shortint qs[$]; + bit [15:0] qu[$]; + + string s; + + initial begin + qs.push_back(-1); + qs.push_back(-2); + qu.push_back(-1); + qu.push_back(-2); + + // Values popped from qs should be treated as signed, values popped from qu + // should be treated as unsigned + s = $sformatf("%0d %0d %0d %0d", qs.pop_front(), qs.pop_back(), + qu.pop_front(), qu.pop_back()); + if (s == "-1 -2 65535 65534") begin + $display("PASSED"); + end else begin + $display("FAILED s=%s", s); + end + end + +endmodule diff --git a/ivtest/ivltests/sv_queue_method_signed3.v b/ivtest/ivltests/sv_queue_method_signed3.v new file mode 100644 index 000000000..95da30275 --- /dev/null +++ b/ivtest/ivltests/sv_queue_method_signed3.v @@ -0,0 +1,99 @@ +// Check that the signedness of the element type of a queue is correctly handled +// whenn calling one of the pop methods with parenthesis. + +module test; + + bit failed = 1'b0; + + `define check(x) \ + if (!(x)) begin \ + $display("FAILED(%0d): ", `__LINE__, `"x`"); \ + failed = 1'b1; \ + end + + int unsigned x = 10; + int y = 10; + int z; + longint w; + + shortint qs[$]; + bit [15:0] qu[$]; + + initial begin + for (int i = 0; i < 16; i++) begin + qu.push_back(-1); + qs.push_back(-1); + end + + // These all evaluate as signed + `check($signed(qu.pop_back) < 0) + `check(qs.pop_back < 0) + + `check($signed(qu.pop_front) < 0) + `check(qs.pop_front < 0) + + // These all evaluate as unsigned + `check(qu.pop_back > 0) + `check({qs.pop_back} > 0) + `check($unsigned(qs.pop_back) > 0) + `check(qs.pop_back > 16'h0) + + `check(qu.pop_front > 0) + `check({qs.pop_front} > 0) + `check($unsigned(qs.pop_front) > 0) + `check(qs.pop_front > 16'h0) + + // In arithmetic expressions if one operand is unsigned all operands are + // considered unsigned + z = qu.pop_back + x; + `check(z === 65545) + z = qu.pop_back + y; + `check(z === 65545) + + z = qu.pop_front + x; + `check(z === 65545) + z = qu.pop_front + y; + `check(z === 65545) + + z = qs.pop_back + x; + `check(z === 65545) + z = qs.pop_back + y; + `check(z === 9) + + z = qs.pop_front + x; + `check(z === 65545) + z = qs.pop_front + y; + `check(z === 9) + + // For ternary operators if one operand is unsigned the result is unsigend + z = x ? qu.pop_back : x; + `check(z === 65535) + z = x ? qu.pop_back : y; + `check(z === 65535) + + z = x ? qu.pop_front : x; + `check(z === 65535) + z = x ? qu.pop_front : y; + `check(z === 65535) + + z = x ? qs.pop_back : x; + `check(z === 65535) + z = x ? qs.pop_back : y; + `check(z === -1) + + z = x ? qs.pop_front : x; + `check(z === 65535) + z = x ? qs.pop_front : y; + `check(z === -1) + + // Size return value is always positive, but check that it gets padded + // properly + w = x ? qu.size : 64'h123; + `check(w === 64'h4) + + if (!failed) begin + $display("PASSED"); + end + end + +endmodule diff --git a/ivtest/ivltests/sv_queue_method_signed4.v b/ivtest/ivltests/sv_queue_method_signed4.v new file mode 100644 index 000000000..b3a3a96a1 --- /dev/null +++ b/ivtest/ivltests/sv_queue_method_signed4.v @@ -0,0 +1,29 @@ +// Check that the signedness of the element type of a queue is correctly handled +// when passing the result of one of the pop methods as an argument to a system +// function. + +module test; + + shortint qs[$]; + bit [15:0] qu[$]; + + string s; + + initial begin + qs.push_back(-1); + qs.push_back(-2); + qu.push_back(-1); + qu.push_back(-2); + + // Values popped from qs should be treated as signed, values popped from qu + // should be treated as unsigned + s = $sformatf("%0d %0d %0d %0d", qs.pop_front, qs.pop_back, + qu.pop_front, qu.pop_back); + if (s == "-1 -2 65535 65534") begin + $display("PASSED"); + end else begin + $display("FAILED s=%s", s); + end + end + +endmodule diff --git a/ivtest/regress-sv.list b/ivtest/regress-sv.list index 888d48339..d1c7b3823 100644 --- a/ivtest/regress-sv.list +++ b/ivtest/regress-sv.list @@ -265,6 +265,10 @@ enum_in_struct normal,-g2005-sv ivltests enum_in_class normal,-g2005-sv ivltests enum_in_class_name_coll CE,-g2005-sv ivltests enum_line_info CE,-g2005-sv ivltests gold=enum_line_info.gold +enum_method_signed1 normal,-g2005-sv ivltests +enum_method_signed2 normal,-g2005-sv ivltests +enum_method_signed3 normal,-g2005-sv ivltests +enum_method_signed4 normal,-g2005-sv ivltests enum_next normal,-g2005-sv ivltests enum_order normal,-g2005-sv ivltests enum_ports normal,-g2005-sv ivltests @@ -488,6 +492,8 @@ sv_class_extends_scoped normal,-g2009 ivltests sv_class_localparam normal,-g2009 ivltests sv_class_new_init normal,-g2009 ivltests sv_class_in_module_decl normal,-g2009 ivltests +sv_class_method_signed1 normal,-g2009 ivltests +sv_class_method_signed2 normal,-g2009 ivltests sv_class_static_prop1 normal,-g2009 ivltests sv_class_static_prop2 normal,-g2009 ivltests sv_class_static_prop3 normal,-g2009 ivltests @@ -571,6 +577,10 @@ sv_queue_parray_fail CE,-g2009 ivltests gold=sv_queue_parray_fail.gold sv_queue_real normal,-g2009,-pfileline=1 ivltests gold=sv_queue_real.gold sv_queue_real_bounded normal,-g2009,-pfileline=1 ivltests gold=sv_queue_real_bounded.gold sv_queue_real_fail CE,-g2009 ivltests gold=sv_queue_real_fail.gold +sv_queue_method_signed1 normal,-g2009 ivltests +sv_queue_method_signed2 normal,-g2009 ivltests +sv_queue_method_signed3 normal,-g2009 ivltests +sv_queue_method_signed4 normal,-g2009 ivltests sv_queue_string normal,-g2009,-pfileline=1 ivltests gold=sv_queue_string.gold sv_queue_string_bounded normal,-g2009,-pfileline=1 ivltests gold=sv_queue_string_bounded.gold sv_queue_string_fail CE,-g2009 ivltests gold=sv_queue_string_fail.gold diff --git a/ivtest/regress-vlog95.list b/ivtest/regress-vlog95.list index 3441f5f6f..55628348e 100644 --- a/ivtest/regress-vlog95.list +++ b/ivtest/regress-vlog95.list @@ -384,6 +384,8 @@ sv_class_extends_scoped CE,-g2009 ivltests sv_class_localparam CE,-g2009 ivltests sv_class_new_init CE,-g2009 ivltests sv_class_in_module_decl CE,-g2009 ivltests +sv_class_method_signed1 CE,-g2009,-pallowsigned=1 ivltests +sv_class_method_signed2 CE,-g2009,-pallowsigned=1 ivltests sv_class_static_prop1 CE,-g2009 ivltests sv_class_static_prop2 CE,-g2009 ivltests sv_class_static_prop3 CE,-g2009 ivltests @@ -438,6 +440,10 @@ br_gh436 CE,-g2012,-pallowsigned=1 ivltests # queues/strings br_gh672 CE,-g2009 ivltests # join_none br_mw20200501 CE,-g2009 ivltests # queues disable_fork_cmd CE,-g2009 ivltests # disable fork and join_* +enum_method_signed1 CE,-g2009,-pallowsigned=1 ivltests +enum_method_signed2 CE,-g2009,-pallowsigned=1 ivltests +enum_method_signed3 CE,-g2009,-pallowsigned=1 ivltests +enum_method_signed4 CE,-g2009,-pallowsigned=1 ivltests enum_next CE,-g2009,-pallowsigned=1 ivltests # enum enum_test1 CE,-g2009 ivltests # enum fork_join_any CE,-g2009,-pallowsigned=1 ivltests # join_any @@ -611,6 +617,10 @@ br_gh433 CE,-g2009,-pallowsigned=1 ivltests sv_queue1 CE,-g2009,-pallowsigned=1 ivltests sv_queue2 CE,-g2009,-pallowsigned=1 ivltests sv_queue3 CE,-g2009 ivltests +sv_queue_method_signed1 CE,-g2009,-pallowsigned=1 ivltests +sv_queue_method_signed2 CE,-g2009,-pallowsigned=1 ivltests +sv_queue_method_signed3 CE,-g2009,-pallowsigned=1 ivltests +sv_queue_method_signed4 CE,-g2009,-pallowsigned=1 ivltests sv_queue_real CE,-g2009 ivltests sv_queue_real_bounded CE,-g2009 ivltests sv_queue_real_fail CE,-g2009 ivltests From 031fbac5be92e57113ca6cebf6aab504ef6cf5ac Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Thu, 7 Apr 2022 11:52:56 +0200 Subject: [PATCH 7/7] Add regression tests for signed class properties Check that the signedness of class properties is handled correctly * When sign extending * When passing as a value to a system function Check this for both when accessing the property from within a class method as well as accessing it on a class object. Signed-off-by: Lars-Peter Clausen --- ivtest/ivltests/sv_class_property_signed1.v | 66 +++++++++++++++++++ ivtest/ivltests/sv_class_property_signed2.v | 25 ++++++++ ivtest/ivltests/sv_class_property_signed3.v | 71 +++++++++++++++++++++ ivtest/ivltests/sv_class_property_signed4.v | 30 +++++++++ ivtest/regress-sv.list | 4 ++ ivtest/regress-vlog95.list | 4 ++ 6 files changed, 200 insertions(+) create mode 100644 ivtest/ivltests/sv_class_property_signed1.v create mode 100644 ivtest/ivltests/sv_class_property_signed2.v create mode 100644 ivtest/ivltests/sv_class_property_signed3.v create mode 100644 ivtest/ivltests/sv_class_property_signed4.v diff --git a/ivtest/ivltests/sv_class_property_signed1.v b/ivtest/ivltests/sv_class_property_signed1.v new file mode 100644 index 000000000..b59f2973c --- /dev/null +++ b/ivtest/ivltests/sv_class_property_signed1.v @@ -0,0 +1,66 @@ +// Check that the signedness of class properties are handled correctly when +// accessing the property on a class object. + +module test; + + bit failed = 1'b0; + + `define check(x) \ + if (!(x)) begin \ + $display("FAILED(%0d): ", `__LINE__, `"x`"); \ + failed = 1'b1; \ + end + + class C; + shortint s = -1; + bit [15:0] u = -1; + endclass + + C c; + + int unsigned x = 10; + int y = 10; + int z; + + initial begin + c = new; + + // These all evaluate as signed + `check(c.s < 0) + `check($signed(c.u) < 0) + + // These all evaluate as unsigned + `check(c.u > 0) + `check({c.s} > 0) + `check($unsigned(c.s) > 0) + `check(c.s > 16'h0) + + // In arithmetic expressions if one operand is unsigned all operands are + // considered unsigned + z = c.u + x; + `check(z === 65545) + z = c.u + y; + `check(z === 65545) + + z = c.s + x; + `check(z === 65545) + z = c.s + y; + `check(z === 9) + + // For ternary operators if one operand is unsigned the result is unsigend + z = x ? c.u : x; + `check(z === 65535) + z = x ? c.u : y; + `check(z === 65535) + + z = x ? c.s : x; + `check(z === 65535) + z = x ? c.s : y; + `check(z === -1) + + if (!failed) begin + $display("PASSED"); + end + end + +endmodule diff --git a/ivtest/ivltests/sv_class_property_signed2.v b/ivtest/ivltests/sv_class_property_signed2.v new file mode 100644 index 000000000..01745dcd2 --- /dev/null +++ b/ivtest/ivltests/sv_class_property_signed2.v @@ -0,0 +1,25 @@ +// Check that the signedness of class properties are handled correctly when +// accessing the property on a class object and passing it to a system function. + +module test; + + class C; + shortint s = -1; + bit [15:0] u = -1; + endclass + + C c; + string s; + + initial begin + c = new; + + s = $sformatf("%0d %0d", c.s, c.u); + if (s == "-1 65535") begin + $display("PASSED"); + end else begin + $display("FAILED s=%s", s); + end + end + +endmodule diff --git a/ivtest/ivltests/sv_class_property_signed3.v b/ivtest/ivltests/sv_class_property_signed3.v new file mode 100644 index 000000000..9b4a81f5e --- /dev/null +++ b/ivtest/ivltests/sv_class_property_signed3.v @@ -0,0 +1,71 @@ +// Check that the signedness of class properties are handled correctly when +// accessing the property in a class method. + +module test; + + bit failed = 1'b0; + + `define check(x) \ + if (!(x)) begin \ + $display("FAILED: ", `"x`", `__LINE__); \ + failed = 1'b1; \ + end + + int unsigned x = 10; + int y = 10; + int z; + + class C; + shortint s = -1; + bit [15:0] u = -1; + + task test; + + // These all evaluate as signed + `check(s < 0) + `check($signed(u) < 0) + + // These all evaluate as unsigned + `check(u > 0) + `check({s} > 0) + `check($unsigned(s) > 0) + `check(s > 16'h0) + + // In arithmetic expressions if one operand is unsigned all operands are + // considered unsigned + z = u + x; + `check(z === 65545) + z = u + y; + `check(z === 65545) + + z = s + x; + `check(z === 65545) + z = s + y; + `check(z === 9) + + // For ternary operators if one operand is unsigned the result is unsigend + z = x ? u : x; + `check(z === 65535) + z = x ? u : y; + `check(z === 65535) + + z = x ? s : x; + `check(z === 65535) + z = x ? s : y; + `check(z === -1) + + if (!failed) begin + $display("PASSED"); + end + endtask + + endclass + + C c; + + initial begin + c = new; + c.test(); + end + +endmodule diff --git a/ivtest/ivltests/sv_class_property_signed4.v b/ivtest/ivltests/sv_class_property_signed4.v new file mode 100644 index 000000000..2d636861b --- /dev/null +++ b/ivtest/ivltests/sv_class_property_signed4.v @@ -0,0 +1,30 @@ +// Check that the signedness of class properties are handled correctly when +// accessing the property in a class method and passing it to a system function. + +module test; + + class C; + shortint s = -1; + bit [15:0] u = -1; + + task test; + string str; + + str = $sformatf("%0d %0d", s, u); + if (str == "-1 65535") begin + $display("PASSED"); + end else begin + $display("FAILED s=%s", s); + end + endtask + + endclass + + C c; + + initial begin + c = new; + c.test(); + end + +endmodule diff --git a/ivtest/regress-sv.list b/ivtest/regress-sv.list index d1c7b3823..e0503d23c 100644 --- a/ivtest/regress-sv.list +++ b/ivtest/regress-sv.list @@ -494,6 +494,10 @@ sv_class_new_init normal,-g2009 ivltests sv_class_in_module_decl normal,-g2009 ivltests sv_class_method_signed1 normal,-g2009 ivltests sv_class_method_signed2 normal,-g2009 ivltests +sv_class_property_signed1 normal,-g2009 ivltests +sv_class_property_signed2 normal,-g2009 ivltests +sv_class_property_signed3 normal,-g2009 ivltests +sv_class_property_signed4 normal,-g2009 ivltests sv_class_static_prop1 normal,-g2009 ivltests sv_class_static_prop2 normal,-g2009 ivltests sv_class_static_prop3 normal,-g2009 ivltests diff --git a/ivtest/regress-vlog95.list b/ivtest/regress-vlog95.list index 55628348e..7aa9989e8 100644 --- a/ivtest/regress-vlog95.list +++ b/ivtest/regress-vlog95.list @@ -386,6 +386,10 @@ sv_class_new_init CE,-g2009 ivltests sv_class_in_module_decl CE,-g2009 ivltests sv_class_method_signed1 CE,-g2009,-pallowsigned=1 ivltests sv_class_method_signed2 CE,-g2009,-pallowsigned=1 ivltests +sv_class_property_signed1 CE,-g2009,-pallowsigned=1 ivltests +sv_class_property_signed2 CE,-g2009,-pallowsigned=1 ivltests +sv_class_property_signed3 CE,-g2009,-pallowsigned=1 ivltests +sv_class_property_signed4 CE,-g2009,-pallowsigned=1 ivltests sv_class_static_prop1 CE,-g2009 ivltests sv_class_static_prop2 CE,-g2009 ivltests sv_class_static_prop3 CE,-g2009 ivltests