operator ordering changes number of ACIR opcodes

[zac-williamson]

Aim

When defining a multiplication, I noticed that the location of a negative sign changed the number of ACIR opcodes. An unneccessary expression is created that simply multiplies a witness by a constant.

Expected Behavior

I expect both versions of the add function (see Bug description) to produce 7 ACIR opcodes. However the second version produces 8 ACIR opcodes due to the unneccessary constraint that is added to negate \lambda

Bug

See how changing the location of the - sign on line 31 changes the compiled ACIR.

To Reproduce

1. clone git@github.com:zac-williamson/noir-edwards.git
2. run nargo info
3. in curve.nr, modify line 31 to be let y_lhs = y * lambda * -d + y + x1x2;
4. run nargo info, observe ACIR constraint count has decreased

Project Impact

None

Impact Context

No response

Workaround

Yes

Workaround Description

Keep fiddling with operator ordering until something works. Rather annoying though.

Additional Context

No response

Installation Method

Binary (noirup default)

Nargo Version

nargo version 0.23

NoirJS Version

0.23

Would you like to submit a PR for this Issue?

None

Support Needs

No response

[TomAFrench]

My guess on this is that the negation of lambda results in us turning a the AcirVarData::Witness for lambda into an AcirVarData::Expr for -lambda, this probably then results in us going down a different codepath for the later multiplications resulting in worse codegen.

Staring at the mul_var method in acir_gen will probably solve this.

[TomAFrench]

On the latest nightly I get 7 opcodes per iteration on both versions.

[TomAFrench]

I'm going to close this issue as it seems to be addressed in master. I know you were working on an older version of nargo yesterday so maybe it's something we fixed in the past?

In any case, feel free to reopen if you can reproduce this on the nightly version of nargo.