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.

Sign up for GitHub

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

systemverilog-plugin: fix handling large constants #465

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
+61 −25
Draft

systemverilog-plugin: fix handling large constants #465

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d5da74d
systemverilog-plugin: make sure size of constant handled by mkconst_i…
kamilrakoczy Mar 6, 2023
871857e
systemverilog-plugin: handle module names when parameter value is lar…
kamilrakoczy Mar 6, 2023
56b7b3b
systemverilog-plugin: apply format
kamilrakoczy Mar 6, 2023
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
86 changes: 61 additions & 25 deletions systemverilog-plugin/UhdmAst.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1236,6 +1236,8 @@ AST::AstNode *UhdmAst::process_value(vpiHandle obj_h)
vpi_get_value(obj_h, &val);
std::string strValType = "'";
bool is_signed = false;
bool is_unsized = false;
int size = -1;
if (vpiHandle typespec_h = vpi_handle(vpiTypespec, obj_h)) {
is_signed = vpi_get(vpiSigned, typespec_h);
if (is_signed) {
Expand Down Expand Up @@ -1274,36 +1276,41 @@ AST::AstNode *UhdmAst::process_value(vpiHandle obj_h)
}
[[fallthrough]];
Comment on lines 1276 to 1277
Copy link
Contributor

Choose a reason for hiding this comment

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

What about case vpiUIntVal? Should it be left as is, or does it need a similar size check as vpiIntVal?

case vpiIntVal: {
if (val.value.integer > std::numeric_limits<std::int32_t>::max()) {
strValType = "'sd";
string str_value = std::to_string(val.value.integer);
val.value.str = strdup(str_value.c_str());
break;
}

int size = -1;
// Surelog sometimes report size as part of vpiTypespec (e.g. int_typespec)
// if it is the case, we need to set size to the left_range of first packed range
visit_one_to_one({vpiTypespec}, obj_h, [&](AST::AstNode *node) {
if (node && node->attributes.count(UhdmAst::packed_ranges()) && node->attributes[UhdmAst::packed_ranges()]->children.size() &&
if (node->attributes.count(UhdmAst::packed_ranges()) && node->attributes[UhdmAst::packed_ranges()]->children.size() &&
kamilrakoczy marked this conversation as resolved.
Show resolved Hide resolved
node->attributes[UhdmAst::packed_ranges()]->children[0]->children.size()) {
size = node->attributes[UhdmAst::packed_ranges()]->children[0]->children[0]->integer + 1;
}
});
if (size == -1) {
size = vpi_get(vpiSize, obj_h);
}
Comment on lines 1287 to 1289
Copy link
Contributor

Choose a reason for hiding this comment

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

Condition always true, size is not changed between initialization and this point.
To make it more visible, let's make size local to this case's scope.

// Surelog by default returns 64 bit numbers and stardard says that they shall be at least 32bits
// yosys is assuming that int/uint is 32 bit, so we are setting here correct size
// NOTE: it *shouldn't* break on explicite 64 bit const values, as they *should* be handled
// above by vpi*StrVal
if (size == 64) {
// we know that value can be stored in 32bit integer
// and yosys only supports 32 bits
size = 32;
is_signed = true;
}
auto c = AST::AstNode::mkconst_int(val.format == vpiUIntVal ? val.value.uint : val.value.integer, is_signed, size > 0 ? size : 32);
if (size == 0 || size == -1)
c->is_unsized = true;
is_signed = true;
if (size == -1) {
is_unsized = false;
size = 32;
}
Comment on lines +1296 to +1299
Copy link
Contributor

Choose a reason for hiding this comment

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

size has been set above to the result of vpi_get(vpiSize, obj_h);. In what situation is it equal to -1?
Please answer in a code comment.

Comment on lines +1296 to +1299
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this if just below the size = vpi_get(vpiSize, obj_h); above?

if (val.value.integer > std::numeric_limits<std::int32_t>::max() || size > 32) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add log_assert(size <= 64) above this. Or comment when this can be the case.

strValType = "'sd";
string str_value = std::to_string(val.value.integer);
val.value.str = strdup(str_value.c_str());
break;
}
if (size > 32) {
const uhdm_handle *const handle = (const uhdm_handle *)obj_h;
const UHDM::BaseClass *const object = (const UHDM::BaseClass *)handle->object;
report_error("%.*s:%d: Constant with size: %d bits handled by mkconst_int that doesn't work with values larger than 32bits\n!",
(int)object->VpiFile().length(), object->VpiFile().data(), object->VpiLineNo(), size);
}
Comment on lines +1306 to +1311
Copy link
Contributor

Choose a reason for hiding this comment

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

Unreachable code. There is a break above when size > 32.

auto c = AST::AstNode::mkconst_int(val.format == vpiUIntVal ? val.value.uint : val.value.integer, is_signed, size);
c->is_unsized = is_unsized;
return c;
}
case vpiRealVal:
Expand Down Expand Up @@ -1345,7 +1352,7 @@ AST::AstNode *UhdmAst::process_value(vpiHandle obj_h)
if (std::strchr(val.value.str, '\'')) {
return ::systemverilog_plugin::const2ast(val.value.str, caseType, false);
} else {
auto size = vpi_get(vpiSize, obj_h);
size = vpi_get(vpiSize, obj_h);
Comment on lines -1348 to +1355
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make the previous size local as mentioned above, and keep this one local too.
And while at it: const int size = ... :)

if (size == 0) {
auto c = AST::AstNode::mkconst_int(atoi(val.value.str), true, 32);
c->is_unsized = true;
Expand Down Expand Up @@ -1891,11 +1898,39 @@ void UhdmAst::process_module()
}
}
if (shared.top_nodes.count(type)) {
if (!node->children[0]->str.empty())
if (!node->children[0]->str.empty()) {
module_parameters += node->str + "=" + node->children[0]->str;
else
} else if (node->children[0]->bits.size() > 32) {
std::string value = std::to_string(node->children[0]->bits.size()) + "'b";
for (const auto &bit : node->children[0]->bits) {
switch (bit) {
case RTLIL::State::S0:
value += "0";
break;
case RTLIL::State::S1:
value += "1";
break;
case RTLIL::State::Sx:
value += "x";
break;
case RTLIL::State::Sz:
value += "z";
break;
case RTLIL::State::Sa:
value += "a";
break;
case RTLIL::State::Sm:
value += "m";
break;
default:
report_error("Unhandled RTLIL State case!\n");
kamilrakoczy marked this conversation as resolved.
Show resolved Hide resolved
}
}
module_parameters += node->str + "=" + value;
} else {
module_parameters +=
node->str + "=" + std::to_string(node->children[0]->bits.size()) + "'d" + std::to_string(node->children[0]->integer);
}
Comment on lines +1903 to +1933
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it follow the way Yosys does it in serialize_param_value?

I think you can even replace this whole snippet (and a bit above and below) with AstNode::derived_module_name + AstNode::asParaConst.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't know about this functions, I've extracted this change to separate PR: #466

}
delete node;
}
Expand Down Expand Up @@ -4016,13 +4051,14 @@ void UhdmAst::process_string_typespec()

void UhdmAst::process_bit_typespec()
{
std::vector<AST::AstNode *> packed_ranges; // comes before wire name
current_node = make_ast_node(AST::AST_WIRE);
visit_range(obj_h, [&](AST::AstNode *node) {
if (node) {
current_node->children.push_back(node);
}
});
visit_range(obj_h, [&](AST::AstNode *node) { packed_ranges.push_back(node); });
Copy link
Contributor

Choose a reason for hiding this comment

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

log_assert(node); or if (node), depending which one is appropriate here.

if (packed_ranges.empty()) {
packed_ranges.push_back(make_range(0, 0));
}
current_node->is_signed = vpi_get(vpiSigned, obj_h);
add_multirange_wire(current_node, packed_ranges, {});
}

void UhdmAst::process_repeat()
Expand Down