Skip to content

Factor out computation of maximum union member #5251

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 1 commit into from
Mar 3, 2020
Merged
Changes from all commits
Commits
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
87 changes: 47 additions & 40 deletions src/solvers/lowering/byte_operators.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,41 @@ Author: Daniel Kroening, [email protected]
#include <util/simplify_expr.h>
#include <util/string_constant.h>

/// Determine the member of maximum fixed bit width in a union type. If no
/// member, or no member of fixed and non-zero width can be found, return
/// nullopt.
/// \param union_type: Type to determine the member of.
/// \param ns: Namespace to resolve tag types.
/// \return Pair of a componentt pointing to the maximum fixed bit-width
/// member of \p union_type and the bit width of that member.
static optionalt<std::pair<struct_union_typet::componentt, mp_integer>>
find_widest_union_component(const union_typet &union_type, const namespacet &ns)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any particular reason this returns an optional pair, rather than just an optional componentt ? Given that the componentt has the the appropriate type in it and the caller could just do pointer_offset_bits themselves. If it's just to simplify future clients, then fine :-) But just wondering as I don't see any current clients using the second half of the pair, and having the extra ->first or ->second slightly reduces readability at the call sites.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that second part is only used in #5252. I hope that's ok - I'll go ahead and merge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FWIW, you're of course right that the caller could compute the size themselves, but I thought duplicating work was not a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine by me. I was reviewing in PR order so hadn't seen the followup PR at that point.

{
const union_typet::componentst &components = union_type.components();

mp_integer max_width = 0;
typet max_comp_type;
irep_idt max_comp_name;

for(const auto &comp : components)
{
auto element_width = pointer_offset_bits(comp.type(), ns);

if(!element_width.has_value() || *element_width <= max_width)
continue;

max_width = *element_width;
max_comp_type = comp.type();
max_comp_name = comp.get_name();
}

if(max_width == 0)
return {};
else
return std::make_pair(
struct_union_typet::componentt{max_comp_name, max_comp_type}, max_width);
}

static exprt bv_to_expr(
const exprt &bitvector_expr,
const typet &target_type,
Expand Down Expand Up @@ -1124,31 +1159,18 @@ exprt lower_byte_extract(const byte_extract_exprt &src, const namespacet &ns)
else if(src.type().id() == ID_union || src.type().id() == ID_union_tag)
{
const union_typet &union_type = to_union_type(ns.follow(src.type()));
const union_typet::componentst &components = union_type.components();

mp_integer max_width = 0;
typet max_comp_type;
irep_idt max_comp_name;

for(const auto &comp : components)
{
auto element_width = pointer_offset_bits(comp.type(), ns);

if(!element_width.has_value() || *element_width <= max_width)
continue;
const auto widest_member = find_widest_union_component(union_type, ns);

max_width = *element_width;
max_comp_type = comp.type();
max_comp_name = comp.get_name();
}

if(max_width > 0)
if(widest_member.has_value())
{
byte_extract_exprt tmp(unpacked);
tmp.type() = max_comp_type;
tmp.type() = widest_member->first.type();

return union_exprt(
max_comp_name, lower_byte_extract(tmp, ns), src.type());
widest_member->first.get_name(),
lower_byte_extract(tmp, ns),
src.type());
}
}

Expand Down Expand Up @@ -1945,34 +1967,19 @@ static exprt lower_byte_update_union(
const optionalt<exprt> &non_const_update_bound,
const namespacet &ns)
{
const union_typet::componentst &components = union_type.components();

mp_integer max_width = 0;
typet max_comp_type;
irep_idt max_comp_name;

for(const auto &comp : components)
{
auto element_width = pointer_offset_bits(comp.type(), ns);

if(!element_width.has_value() || *element_width <= max_width)
continue;

max_width = *element_width;
max_comp_type = comp.type();
max_comp_name = comp.get_name();
}
const auto widest_member = find_widest_union_component(union_type, ns);

PRECONDITION_WITH_DIAGNOSTICS(
max_width > 0,
widest_member.has_value(),
"lower_byte_update of union of unknown size is not supported");

byte_update_exprt bu = src;
bu.set_op(member_exprt{src.op(), max_comp_name, max_comp_type});
bu.type() = max_comp_type;
bu.set_op(member_exprt{
src.op(), widest_member->first.get_name(), widest_member->first.type()});
bu.type() = widest_member->first.type();

return union_exprt{
max_comp_name,
widest_member->first.get_name(),
lower_byte_update(bu, value_as_byte_array, non_const_update_bound, ns),
src.type()};
}
Expand Down