Skip to content

fix(ssa): Check cast bit size when fetching the max num bits for a value #4699

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

Closed
vezenovm wants to merge 4 commits into from
Closed

fix(ssa): Check cast bit size when fetching the max num bits for a value #4699

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
262b9ca
make more granular usage of a cast bit size in get_value_max_num_bits
vezenovm Apr 2, 2024
3b6d107
remove debug
vezenovm Apr 2, 2024
ef0a5ad
use bit_size()
vezenovm Apr 2, 2024
1e82b87
back to get_value_max_num_bits and improve comment
vezenovm Apr 3, 2024
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
10 changes: 8 additions & 2 deletions compiler/noirc_evaluator/src/ssa/ir/dfg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,10 +344,16 @@ impl DataFlowGraph {
pub(crate) fn get_value_max_num_bits(&self, value: ValueId) -> u32 {
match self[value] {
Value::Instruction { instruction, .. } => {
let bit_size = self.type_of_value(value).bit_size();
if let Instruction::Cast(original_value, _) = self[instruction] {
self.type_of_value(original_value).bit_size()
let original_bit_size = self.type_of_value(original_value).bit_size();
if original_bit_size < bit_size {
original_bit_size
} else {
bit_size
}
} else {
self.type_of_value(value).bit_size()
bit_size
}
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ impl Binary {
let non_constant = if lhs.is_some() { self.rhs } else { self.lhs };
if let Some(constant) = constant {
let max_possible_value =
2u128.pow(dfg.get_value_max_num_bits(non_constant)) - 1;
2u128.pow(dfg.type_of_value(non_constant).bit_size()) - 1;
Copy link
Copy Markdown
Contributor Author

@vezenovm vezenovm Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There is actually two issues. The Cast logic was incorrect and also that in #4691 we should be checking against the bit_size of the variable's type. According to the comments for get_value_max_num_bits it is attempting to the following, which is not what we want:

Returns the maximum possible number of bits that `value` can potentially be

Copy link
Copy Markdown
Contributor Author

@vezenovm vezenovm Apr 2, 2024

Choose a reason for hiding this comment

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

I'll pull this out into its own fix and we can wait for @TomAFrench to take a look at the Cast fix as those are his changes.

Copy link
Copy Markdown
Member

@TomAFrench TomAFrench Apr 2, 2024

Choose a reason for hiding this comment

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

Can you explain why we should make this change? The motivation for the current implementation is that get_value_max_num_bits will return the max number of bits which value can take, i.e. non_constant < 2^max_bits. This allows us to reason how about when the constant is greater than this upper bound then it's impossible for the equality to hold.

This relies on casts up from a smaller type as otherwise the constant will always be less than the max potential value of the variable.

Copy link
Copy Markdown
Contributor Author

@vezenovm vezenovm Apr 2, 2024

Choose a reason for hiding this comment

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

This relies on casts up from a smaller type as otherwise the constant will always be less than the max potential value of the variable.

Ok I understand thank you for elaborating. I was thinking that using get_value_max_num_bits was a typo due to the comments above about variable types. I can update those to discuss that we want to check how for casts from a lower type as well as bringing usage back of get_value_max_num_bits.

if constant > max_possible_value.into() {
let zero = dfg.make_constant(FieldElement::zero(), Type::bool());
return SimplifyResult::SimplifiedTo(zero);
Expand Down