Skip to content

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Sep 3, 2025

Description

Problem*

Trying to find pieces of logic not covered by unit or integration tests.

Summary*

Add more unit test coverage to LICM.

Refactor and fix some stuff:

  • Remove Constrain and RangeCheck from can_be_hoisted_from_loop_bounds: they don't look at the bounds, and the conditions they check are redundant with can_be_hoisted.
  • Remove Call from can_be_hoisted_from_loop_bounds: same reason as above.
  • Remove BlockContext::can_be_hoisted_with_control_dependence: I thought the extra jumping around to call can_be_hoisted again with true was confusing, and it was clearer to see the calls to can_be_hoisted with false and true next to each other, with the latter being restricted by block_context.can_hoist_control_dependent_instruction().
  • Changed can_be_hoisted to return Yes | No | WithPredicate, rather than having to call it twice with false and true; this way if it returns No we don't have to retry in futility, and I think it reads clearer.
  • Return false for loops without an exit block from the header in is_fully_executed. It used return true, but two examples I found were 1) an actual infinite loop in regression_7103 and 2) a while true loop with a break created by the fuzzer to avoid infinite loops. The latter is clearly not in the spirit of the method, as it has a break.
  • Handle loops with headers that have no invariant parameter in get_const_upper_bound, e.g. while <bool-variable>.
  • Consider the loop header as an ancestor of the blocks in the loop for calculating is_impure.
  • Fix the simplification of ConstrainNotEqual

Untested code

The following changes don't cause any test failure:

  • can_be_hoisted_from_loop_bounds:
    • ArrayGet can be disabled
    • ArrayGet can return true instead of false
    • Constrain(..) | ConstrainNotEqual(..) | RangeCheck { .. } can be disabled
    • Call { func, .. } can be disabled
  • can_be_hoisted:
    • Call / Purity::Pure: return false instead of true
    • Call / Purity::PureWithPredicate: return false instead of hoist_with_predicate
    • Call / purity None: return true instead of false
    • MakeArray: return true or false instead of function.runtime().is_acir()
  • can_evaluate_binary_op:
    • Add | Mul | Sub when unchecked: true: return false instead of true
    • Div | Mod: return false instead of true when !value.is_zero()
  • simplify_from_loop_bounds:
    • SimplifyResult::Remove: return false instead of true
  • simplify_induction_variable:
    • Constrain: remove loop_context.no_break from condition
    • ConstrainNotEqual:
      • remove loop_context.no_break from condition
      • remove block_context.can_simplify_control_dependent_instruction() from condition
      • disable altogether
  • simplify_induction_variable_in_binary:
    • false if upper_bound <= constant.inc() if we remove .inc()
    • false if lower_bound > constant if we return false_value instead of true_value
    • true if lower_bound >= constant if we return true_value instead of false_value
    • false if upper_bound <= constant.inc() if we return true_value instead of false_value

This is not an exhaustive list, just what I found by tinkering with conditions.

I'll open follow up PRs to add tests, but wanted to get this PR reviewed because of the refactoring it contains.

Additional Context

I used the following commands to look for failing tests:

cargo nextest run --cargo-quiet -p noirc_evaluator loop_invariant
cargo nextest run --cargo-quiet -p nargo_cli --test execute forcebrillig_false_inliner_i64_min --filter-expr "not test(ram_blowup)"

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.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ 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: 60d09ad Previous: 8d5ae13 Ratio
test_report_zkpassport_noir-ecdsa_ 2 s 1 s 2

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

CC: @TomAFrench

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ 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: 2be3e26 Previous: 8d5ae13 Ratio
private-kernel-inner 0.019 s 0.014 s 1.36

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

CC: @TomAFrench

@aakoshh aakoshh changed the title chore(licm): Add more unit tests chore(licm): More unit tests and refactoring Sep 3, 2025
@aakoshh aakoshh changed the title chore(licm): More unit tests and refactoring chore(licm): Identify untested code; refactoring; minor fix Sep 3, 2025
@aakoshh aakoshh marked this pull request as ready for review September 3, 2025 21:17
@aakoshh aakoshh requested a review from a team September 3, 2025 21:20
@aakoshh aakoshh changed the title chore(licm): Identify untested code; refactoring; minor fix chore(licm): Identify untested code; refactoring; minor fixes Sep 4, 2025
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

LGTM

@aakoshh aakoshh enabled auto-merge September 5, 2025 12:09
@aakoshh aakoshh added this pull request to the merge queue Sep 5, 2025
Merged via the queue into master with commit 14659f3 Sep 5, 2025
122 checks passed
@aakoshh aakoshh deleted the af/licm-tests branch September 5, 2025 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants