Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

fix: maintain Opcode ordering in CSATTransformer #419

Closed
wants to merge 1 commit into from

Conversation

TomAFrench
Copy link
Member

Description

Problem*

Resolves

Summary*

This PR stops us from sorting the set of arithmetic opcodes produced by the CSATTransformer.

Consider the arithmetic opcode

EXPR [ (1, _1, _7) (1, _5, _8) (-1, _1) (-1, _9) -1 ]

This currently gets transformed into

EXPR [ (1, _1, _7) (-1, _13) 0 ]
EXPR [ (1, _5, _8) (-1, _14) 0 ]
EXPR [ (-1, _1) (-1, _9) (1, _15) -1 ]
EXPR [ (1, _13) (1, _14) (-1, _15) 0 ]

The 3rd and 4th opcodes have been swapped as the original opcode's linear term gets it sorted before the final intermediate value gate. If we don't sort the gates then we get the result

EXPR [ (1, _1, _7) (-1, _13) 0 ]
EXPR [ (1, _5, _8) (-1, _14) 0 ]
EXPR [ (1, _13) (1, _14) (-1, _15) 0 ]
EXPR [ (-1, _1) (-1, _9) (1, _15) -1 ]

This is what we expect as the gate which pulls together all of the intermediate values is at the end so can act once these are set.

Additional Context

PR Checklist*

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

@TomAFrench
Copy link
Member Author

I've maintained the ordering traits on Expression to prevent this from being breaking.

@TomAFrench
Copy link
Member Author

Note that this isn't a silver bullet as we must also ensure that the "output" of the original expression is not placed into an intermediate value noir-lang/noir#1729 (comment)

@TomAFrench
Copy link
Member Author

Closing as this is only a situational fix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant