chore(ACIR): inline maybe_eq_predicate

[asterite]

Description

Problem

No issue, just something I noticed and wanted to try out.

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.

[TomAFrench]

huh, I'd expect that mul_var(var, 0) would return 0 and then we then run assert_eq_var(0, 0 None) which can be resolved at compile time.

[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: e7e7fcf	Previous: 75c7d69	Ratio
test_report_zkpassport_noir-ecdsa_	4 s	2 s	2
test_report_zkpassport_noir_rsa_	3 s	1 s	3

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

CC: @TomAFrench

[asterite]

@TomAFrench I think that's what happens. But if predicate is not 0 we end up with var*predicate = predicate which is more complex than var = predicate... though the meaning is not exactly the same 🤔

In any case, if this just means one less opcode in just a couple of test programs, maybe it's not worth it (I somehow fear this could break something).

[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: f5b6780	Previous: 75c7d69	Ratio
rollup-root	0.005 s	0.004 s	1.25

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

CC: @TomAFrench

[TomAFrench]

Also this is changing the semantics of the constraint. Previously we had predicate * var == predicate --> predicate * (var - 1) == 0 (so we require (predicate == 0) || (var == 1) but now it's var == predicate

[TomAFrench]

Ah yeah, sorry you wrote this in your comment (I should learn to read one of these days).

Yeah, this is meaningful change as we're now no longer turning off the constraint when the predicate is inactive but instead just constraining it to a different value. It's then not working as a predicate should.

[TomAFrench]

I somehow fear this could break something

If it doesn't then we need to add a new test!

[asterite]

That makes sense.

Should we at least change the doc comments of maybe_eq_predicate? I'm not sure they match what the function ends up doing.

[TomAFrench]

Yeah I noticed the doc comment doesn't quite line up with my thinking after I posted. lemme just check the code again.

[TomAFrench]

I've refactored inv_var so that we only call into maybe_eq_predicate once so we can reason about what we need more easily. I think the ideal case of this PR is that we get rid of maybe_eq_predicate entirely and inline it into inv_var.

[TomAFrench]

The potential issue I can see with your refactor as a result of this is:

• in the case where we're inverting a variable then things are fine as the brillig call will output zero due to the predicate being applied to it. should_be_one == predicate is what we want in this case.
• If we're inverting a constant then if it's invertable then we'll have inverted_var as non-zero even under a zero predicate so should_be_one can be 1 and so we end up doing should_be_one == predicate --> 1 == 0.

[TomAFrench]

That said any constant field inversions should be removed in SSA so this would need to be introduced as part of ACIRgen in order for it to cause an issue.