Skip to content

SystemVerilog Unions #560

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

Merged
merged 2 commits into from
Jun 19, 2024
Merged

SystemVerilog Unions #560

merged 2 commits into from
Jun 19, 2024

Conversation

kroening
Copy link
Member

This adds basic support for SystemVerilog unions.

Comment on lines +196 to +212
if(type.id() == ID_union)
{
// find the biggest
mp_integer max = 0;
for(auto &component : to_verilog_union_type(type).components())
max = std::max(max, get_width(component.type()));
return max;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

union_typet::find_widest_union_component should do the trick.

Copy link
Member Author

Choose a reason for hiding this comment

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

That uses C widths, in bytes, whereas we'd need Verilog widths, in bits. That method would need to take a functor that computes the width of a field.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It uses pointer_offset_bits, so should be bits. Might, however, still be a problem that pointer_offset_bits doesn't know all the Verilog types? If so, is ID_union the right type at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I make it verilog_union, then the flattener would need to learn about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So perhaps the better route is to produce an implementation that is similar to union_typet::find_widest_union_component to have almost-the-same-code twice in the Verilog front-end?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll see of those two pieces of code can become one.

Comment on lines 495 to 507
else if(type.id() == ID_union)
{
auto &union_type = to_verilog_union_type(type);
mp_integer max = 0;
for(auto &component : union_type.components())
{
auto component_bits_opt = bits_rec(component.type());
if(!component_bits_opt.has_value())
return component_bits_opt.value();
max = std::max(max, component_bits_opt.value());
}
return max;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should use union_typet::find_widest_union_component.

@kroening kroening marked this pull request as ready for review June 19, 2024 16:25
Comment on lines +196 to +212
if(type.id() == ID_union)
{
// find the biggest
mp_integer max = 0;
for(auto &component : to_verilog_union_type(type).components())
max = std::max(max, get_width(component.type()));
return max;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So perhaps the better route is to produce an implementation that is similar to union_typet::find_widest_union_component to have almost-the-same-code twice in the Verilog front-end?

kroening added 2 commits June 19, 2024 14:08
This replaces verilog_typecheck_exprt::bits_rec by an invocation to
verilog_typecheck_baset::get_width_opt.
This adds basic support for SystemVerilog unions.
@kroening kroening merged commit bd5891b into main Jun 19, 2024
6 checks passed
@kroening kroening deleted the unions branch June 19, 2024 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants