fix: avoid invalid cast in remove_bit_shifts

[asterite]

Description

Problem

Resolves #9546

Summary

Additional Context

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 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: d2c102f	Previous: 4451b45	Ratio
test_report_zkpassport_noir-ecdsa_	3 s	1 s	3

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

CC: @TomAFrench

[TomAFrench]

This truncation shouldn't be necessary afaict. two_pow is limited to returning a value which is guaranteed to fit in the correct number of bits so this truncation is either a noop or is unreachable (as it only truncates in the situation where the bitshift has an overflow.).

It looks like this is just an issue when we have an invalid RHS value which is a constant, could we not inject a failing constraint and a placeholder return value in this case?

[aakoshh]

could we not inject a failing constraint and a placeholder return value in this case?

@TomAFrench if you look at #9546 (comment) then it shows the always failing constraint is indeed injected, but it's followed by a Cast of the untruncated overflowed value. However after the flattening pass the always-fail constraint, which was in a block we may or may not have entered, is transformed into a failure conditional on a variable, and that lets the Cast go through even though side effects are disabled.

A placeholder value instead of the Cast could indeed solve this.

[asterite]

Makes sense, I'll do it 👍

The only downside is that intermediate SSA might not always be valida according to our validate_ssa so we won't be able to easily catch these things by validating intermediate SSA during nargo interpret, but maybe that's fine to be able to produce more optimal code.

[aakoshh]

@asterite as I understand the idea would be not to generate invalid SSA, but to instead replace what is currently a cast without a truncate that we know will fail with a default value that won't fail, so the SSA will stay valid.

Currently the SSA has a stage like this:

After Mem2Reg (step 16):
acir(inline) impure fn main f0 {
  ...
  b3():
    v19 = shr i64 8724006435340102799, i64 4976821465038191667	// src/main.nr:7:20
    v20 = truncate v19 to 64 bits, max_bit_size: 65	// src/main.nr:7:20
    v21 = truncate v20 to 16 bits, max_bit_size: 64	// src/main.nr:7:19
    v22 = cast v21 as i16                         	// src/main.nr:7:19
    v24 = unchecked_add v22, i16 9004             	// src/main.nr:7:18
...

and then it becomes this:

After Removing Bit Shifts (step 17):
acir(inline) impure fn main f0 {
...
  b3():
    constrain u1 0 == u1 1, "attempt to bit-shift with overflow"	// src/main.nr:7:20
    v19 = cast Field -4621201446125995037119335439483945574226800510112916213350370146272101329412 as i64	// src/main.nr:7:20

So remove_bit_shifts evaluated the shr and inserted this cast, but it did not consider that this would be an invalid value according to validate_ssa. We even had truncate before. On the positive side it did insert the constrain which will always fail. Whatever did this (insert always-fail constraint, remove truncates and insert the cast wit the result), should just insert v19 = i64 0 instead, because it will never be used due to the always-fail constraint.

[asterite]

The problem is that this SSA:

acir(inline) fn main f0 {
  b0(v0: u32, v1: u32):
    v2 = shr v0, v1
    return v2
}

is transformed to this one by remove_bit_shifts:

acir(inline) fn main f0 {
  b0(v0: u32, v1: u32):
    v3 = lt v1, u32 32
    constrain v3 == u1 1, "attempt to bit-shift with overflow"
    v5 = cast v1 as Field
    v7 = call to_le_bits(v5) -> [u1; 6]
    v9 = array_get v7, index u32 5 -> u1
    v10 = not v9
    v11 = cast v9 as Field
    v12 = cast v10 as Field
    v14 = mul Field 2, v11
    v15 = add v12, v14
    v17 = array_get v7, index u32 4 -> u1
    v18 = not v17
    v19 = cast v17 as Field
    v20 = cast v18 as Field
    v21 = mul v15, v15
    v22 = mul v21, v20
    v23 = mul v21, Field 2
    v24 = mul v23, v19
    v25 = add v22, v24
    v27 = array_get v7, index u32 3 -> u1
    v28 = not v27
    v29 = cast v27 as Field
    v30 = cast v28 as Field
    v31 = mul v25, v25
    v32 = mul v31, v30
    v33 = mul v31, Field 2
    v34 = mul v33, v29
    v35 = add v32, v34
    v37 = array_get v7, index u32 2 -> u1
    v38 = not v37
    v39 = cast v37 as Field
    v40 = cast v38 as Field
    v41 = mul v35, v35
    v42 = mul v41, v40
    v43 = mul v41, Field 2
    v44 = mul v43, v39
    v45 = add v42, v44
    v47 = array_get v7, index u32 1 -> u1
    v48 = not v47
    v49 = cast v47 as Field
    v50 = cast v48 as Field
    v51 = mul v45, v45
    v52 = mul v51, v50
    v53 = mul v51, Field 2
    v54 = mul v53, v49
    v55 = add v52, v54
    v57 = array_get v7, index u32 0 -> u1
    v58 = not v57
    v59 = cast v57 as Field
    v60 = cast v58 as Field
    v61 = mul v55, v55
    v62 = mul v61, v60
    v63 = mul v61, Field 2
    v64 = mul v63, v59
    v65 = add v62, v64
    v66 = cast v65 as u32 // <-- this cast
    v67 = div v0, v66
    return v67
}

At least that's in the unsigned case. I think all of those instructions up until the commented cast is two_pow.

In this case both lhs and rhs are not constant so we can't know if it'll overflow or not.

If we run validate_ssa on the above SSA it fails saying that the commented cast is invalid as it's not preceded by a truncation. However, the cast is guaranteed to never fail because of the constrain above, but validate_ssa can't possibly know that.

[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: 972418f	Previous: 4451b45	Ratio
private-kernel-tail	1.664 s	1.336 s	1.25

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

CC: @TomAFrench

[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: 972418f	Previous: 4451b45	Ratio
private-kernel-tail	0.013 s	0.01 s	1.30

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

CC: @TomAFrench

[asterite]

This is probably related to #8645

[aakoshh]

Ah okay, I see. I misunderstood and thought invalid SSA as in the interpreter will choke on it somehow.

[asterite]

This is now ready for review

[aakoshh]

Looks good!