systemverilog-plugin: fix handling large constants #465
Conversation
…nt is lower than 32bits Signed-off-by: Kamil Rakoczy <[email protected]>
…ger than 32bit Signed-off-by: Kamil Rakoczy <[email protected]>
Signed-off-by: Kamil Rakoczy <[email protected]>
There was a problem hiding this comment.
The whole situation with size in case vpiIntVal is so confusing it should be briefly described in a comment as a list of facts.
For example, in what situation can we expect size > 32 && size < 64? How does this situation differs from a situation when size == 64 (since there's a special case for this value)?
Please also add a test that covers all edge cases for vpiIntVal, i.e. sizes equal to: -1, 0, 1, 32, 33, 63, 64. I ask for a more detailed test here, because we already fixed (not sufficiently) this function before, and the thing the code has to do here is a bit confusing.
A test for module naming would be nice too.
| if (size == -1) { | ||
| is_unsized = false; | ||
| size = 32; | ||
| } |
There was a problem hiding this comment.
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.
| if (size == -1) { | ||
| is_unsized = false; | ||
| size = 32; | ||
| } |
There was a problem hiding this comment.
Can we move this if just below the size = vpi_get(vpiSize, obj_h); above?
| 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); | ||
| } |
There was a problem hiding this comment.
Unreachable code. There is a break above when size > 32.
| if (size == -1) { | ||
| size = vpi_get(vpiSize, obj_h); | ||
| } |
There was a problem hiding this comment.
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.
| is_unsized = false; | ||
| size = 32; | ||
| } | ||
| if (val.value.integer > std::numeric_limits<std::int32_t>::max() || size > 32) { |
There was a problem hiding this comment.
Add log_assert(size <= 64) above this. Or comment when this can be the case.
| auto size = vpi_get(vpiSize, obj_h); | ||
| size = vpi_get(vpiSize, obj_h); |
There was a problem hiding this comment.
Let's make the previous size local as mentioned above, and keep this one local too.
And while at it: const int size = ... :)
| } | ||
| [[fallthrough]]; |
There was a problem hiding this comment.
What about case vpiUIntVal? Should it be left as is, or does it need a similar size check as vpiIntVal?
| } 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"); | ||
| } | ||
| } | ||
| 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); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I didn't know about this functions, I've extracted this change to separate PR: #466
| current_node->children.push_back(node); | ||
| } | ||
| }); | ||
| visit_range(obj_h, [&](AST::AstNode *node) { packed_ranges.push_back(node); }); |
There was a problem hiding this comment.
log_assert(node); or if (node), depending which one is appropriate here.
mkconst_inthandles integers up to 32bits: https://github.com/YosysHQ/yosys/blob/master/frontends/ast/ast.cc#L754.Whole constant can be stored in
bitsfield. Before we were checking if value can be stored inside uint32_t, but size of const could still be larger than 32bits.This PR moves handling of constants that are larger than 32bits to
const2astas well as fixes rename of module when parameter value is larger than 32bit.yosys-systemverilog run: https://github.com/antmicro/yosys-systemverilog/actions/runs/4343198440