fix: Use IntegerConstant for loop boundaries in unrolling

[aakoshh]

Description

Problem*

Resolves #8009

Summary*

Fixes the get_const_lower_bound and get_const_upper_bound methods in the unrolling module to use a new IntegerConstant type, which uses the binary::try_convert_field_element_to_signed_integer function for Signed types to convert them to i128, which can correctly represent negative values for our use case. Unsigned types are converted to u128 instead.

Additional Context

The program in the bug ticket was misleading: -1 % 5 is 4 according to Python, but -1 according to Rust; it was turned into 4294967295 by the as u32 operation, not the modulo. I added the cast because the frontend didn't let me use negative literals in the loop.

The negative result for modulo would not necessarily be a problem for the AST story: if we loop from 3..-1, it's not going to do anything, but that's okay, other times it will. Generating a block to declare a value, and then conditionally add the modulo to it to make it positive seems like extreme overkill (it would be unreadable in AST prints).

The original problem where it turned into 18446744073709551615 did not involve any casting, only a literal in the AST. This literal cannot be used in the for loop in Noir, nor does it allow negatives in the loop, so I added a test directly creating SSA from a hand crafted AST to replicate this. It turned out this is expected behaviour; this number if u64::MAX, which is how negative values of i64 are represented as a Field in the SSA. Special care needs to be taken to convert the value back to signed integers. (EDIT: It turns out it can be used if the bounds are declared separately as variables.)

Documentation*

Check one:

• No documentation needed.
• Documentation included in this PR.
• [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt on default settings.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: a4b3728	Previous: 797aea9	Ratio
private-kernel-inner	0.034 s	0.028 s	1.21

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[TomAFrench]

fyi, we currently allow using u128s in loop bounds which may cause issues.

[aakoshh]

Right. Well currently these two are just optimisations, so hopefully they would just be skipped as None, but we could look at the signedness of the type and return Either<i128, u128> I suppose.

[aakoshh]

I may have crossed my eyes, but I thought I restricted the index to be 64 bits because Noir wouldn’t let me use 128 bits:

noir/tooling/ast_fuzzer/src/program/func.rs

Line 788 in b591847

// The index can be signed or unsigned int, 8 to 64 bits,

[aakoshh]

But you are right, the interpreter handles u128:

noir/compiler/noirc_frontend/src/hir/comptime/interpreter.rs

Line 1238 in b591847

Value::U128(_) => |i| Value::U128(i),

[vezenovm]

LGTM just some minor nits

[vezenovm]

Right. Well currently these two are just optimisations, so hopefully they would just be skipped as None

It does look like as this just used during optimizations we should just return None for u128 range values over i128. So this shouldn't cause any issues, but let's make an issue to follow-up on this (although it will be low priority). Could even have a small unit test that shows an optimization that works will a u32 or u64 bounds but not a certain u128 bound (e.g. like hoist_array_gets_using_induction_variable_with_const_bound or a different test that is using the induction variable bounds).

[aakoshh]

It does look like as this just used during optimizations we should just return None for u128 range values over i128.

@asterite pointed out in #8011 that using u128 range bounds will crash the compiler, which it did. I got confused earlier because of this:

error: The value `170141183460469231731687303715884105715` cannot fit into `u32` which has range `0..=4294967295`
   ┌─ src/main.nr:22:14
   │
22 │     for _ in 170141183460469231731687303715884105715..170141183460469231731687303715884105730 {
   │              ---------------------------------------
   │
   = Call stack:
     1. src/main.nr:22:14

Aborting due to 1 previous error

I forgot that you can still assign to a u128 and it will accept it.

I introduced a new IntegerConstant type which can be either Signed or Unsigned to handle all permissible cases, and updated the unrolling and hoisting to use this type instead of FieldElement for handling bounds.

[vezenovm]

Looks great! I like the new type. I just have some minor nits and one correction regarding comparisons in the new type

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 34cb742	Previous: 20fdf4c	Ratio
rollup-block-root	152 s	119 s	1.28

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[vezenovm]

rollup-block-root 152 s 119 s 1.28

Pretty surprised to see this large of a regression. We do have large loops that need to be unrolled here. Perhaps we are doing a lot of unnecessary conversions?

[aakoshh]

I'll create a follow-up ticket to investigate the slowdown with a profiler: #8168