Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(avm): operands reordering #10182

Merged
merged 4 commits into from
Nov 25, 2024
Merged

chore(avm): operands reordering #10182

merged 4 commits into from
Nov 25, 2024

Conversation

jeanmon
Copy link
Contributor

@jeanmon jeanmon commented Nov 25, 2024

Resolves #10136

@jeanmon jeanmon marked this pull request as ready for review November 25, 2024 15:52
@fcarreiro fcarreiro removed the request for review from IlyasRidhuan November 25, 2024 15:57
fcarreiro
fcarreiro reviewed Nov 25, 2024
View reviewed changes
avm-transpiler/src/instructions.rs
Ok(())
}
}

impl AvmInstruction {
/// Bytes representation for generating AVM bytecode
/// Order: INDIRECT, OPERANDS, TAG, IMMEDIATES
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, in principle I saw no reason to introduce the immediates separately, since you could've just modified the particular opcodes and put the immediate at the end. However then I saw you have the tag in between them. Is this needed? if you do: Indirect, addresses, immediates, tag then I think you wouldn't need to separate the immediates and that seems conceptually right?

Or do you need the tag to be in the middle for some reason?

Copy link
Contributor Author

@jeanmon jeanmon Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@fcarreiro I had the same thoughts and the reason I nevertheless introduced the immediates is for the SET opcode. If we put the tag before the immediates then all set opcode flavors start the same which would help to have less complex relations in bytecode decomposition.

I thought that conceptually to have "immediates" is not too bad neither.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, the offset for TAG in set opcode would be at the same position (except SET_8) for all flavors. If we put TAG at the end, then we need to offset by the different immediate sizes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Ok, not too bad.

barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.hpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that you don't need to make these ones match the serialization. In TS you have to, but here you are ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I have found it convenient for reading purposes but did not perform this for the SET opcode as it would have been too tedious.

@fcarreiro fcarreiro self-requested a review November 25, 2024 16:40
fcarreiro
fcarreiro approved these changes Nov 25, 2024
View reviewed changes
avm-transpiler/src/instructions.rs
Ok(())
}
}

impl AvmInstruction {
/// Bytes representation for generating AVM bytecode
/// Order: INDIRECT, OPERANDS, TAG, IMMEDIATES
Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Ok, not too bad.

barretenberg/cpp/src/barretenberg/vm/avm/trace/deserialization.cpp Outdated Show resolved Hide resolved
@jeanmon jeanmon enabled auto-merge (squash) November 25, 2024 16:48
@jeanmon jeanmon merged commit 69bdf4f into master Nov 25, 2024
71 of 72 checks passed
@jeanmon jeanmon deleted the jm/10136-operands-reordering branch November 25, 2024 17:45
@AztecBot AztecBot mentioned this pull request Nov 25, 2024
Merged
TomAFrench added a commit that referenced this pull request Nov 25, 2024
@TomAFrench
Merge branch 'master' into tf/remove-schnorr
9d841f8 
* master: (106 commits)
  feat: blobs. (#9302)
  chore(avm): operands reordering (#10182)
  feat: UltraRollupRecursiveFlavor (#10088)
  feat: one liner for nodes to join rough-rhino (#10168)
  feat!: rename sharedimmutable methods (#10164)
  chore(master): Release 0.64.0 (#10043)
  feat: e2e metrics reporting (#9776)
  chore: fix pool metrics (#9652)
  chore: Initial draft of testnet-runbook (#10085)
  feat: Improved data storage metrics (#10020)
  chore: Remove handling of duplicates from the note hash tree (#10016)
  fix: add curl to aztec nargo container (#10173)
  fix: Revert "feat: integrate base fee computation into rollup" (#10166)
  feat!: rename SharedMutable methods (#10165)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  chore: replace relative paths to noir-protocol-circuits
  git subrepo push --branch=master barretenberg
  feat: sync tags as sender (#10071)
  feat: integrate base fee computation into rollup (#10076)
  ...
TomAFrench added a commit that referenced this pull request Nov 26, 2024
@TomAFrench
Merge branch 'master' into sync-noir
0911fb1 
* master: (64 commits)
  fix: docker compose aztec up fix (#10197)
  fix: aztec-nargo curl in the earthfile also (#10199)
  chore: fix devbox (#10201)
  chore: misc cleanup (#10194)
  fix: release l1-contracts (#10095)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  chore: replace relative paths to noir-protocol-circuits
  git subrepo push --branch=master barretenberg
  feat: Origin tags implemented in biggroup (#10002)
  fix: Revert "feat: blobs. (#9302)" (#10187)
  feat!: remove SharedImmutable (#10183)
  fix(bb.js): don't minify bb.js - webpack config (#10170)
  feat: blobs. (#9302)
  chore(avm): operands reordering (#10182)
  feat: UltraRollupRecursiveFlavor (#10088)
  feat: one liner for nodes to join rough-rhino (#10168)
  feat!: rename sharedimmutable methods (#10164)
  chore(master): Release 0.64.0 (#10043)
  feat: e2e metrics reporting (#9776)
  ...
critesjosh pushed a commit that referenced this pull request Nov 26, 2024
@AztecBot
chore(master): Release 0.65.0 (#10181)
903bcb0 
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-package: 0.65.0</summary>

##
[0.65.0](aztec-package-v0.64.0...aztec-package-v0.65.0)
(2024-11-26)


### Features

* **avm:** New public inputs witgen
([#10179](#10179))
([ac8f13e](ac8f13e))
</details>

<details><summary>barretenberg.js: 0.65.0</summary>

##
[0.65.0](barretenberg.js-v0.64.0...barretenberg.js-v0.65.0)
(2024-11-26)


### Bug Fixes

* **bb.js:** Don't minify bb.js - webpack config
([#10170](#10170))
([6e7fae7](6e7fae7))
</details>

<details><summary>aztec-packages: 0.65.0</summary>

##
[0.65.0](aztec-packages-v0.64.0...aztec-packages-v0.65.0)
(2024-11-26)


### ⚠ BREAKING CHANGES

* remove SharedImmutable
([#10183](#10183))
* rename sharedimmutable methods
([#10164](#10164))

### Features

* **avm:** New public inputs witgen
([#10179](#10179))
([ac8f13e](ac8f13e))
* Blobs.
([#9302](#9302))
([03b7e0e](03b7e0e))
* One liner for nodes to join rough-rhino
([#10168](#10168))
([3a425e9](3a425e9))
* Origin tags implemented in biggroup
([#10002](#10002))
([c8696b1](c8696b1))
* Remove SharedImmutable
([#10183](#10183))
([a9f3b5f](a9f3b5f))
* Rename sharedimmutable methods
([#10164](#10164))
([ef7cd86](ef7cd86))
* UltraRollupRecursiveFlavor
([#10088](#10088))
([4418ef2](4418ef2))


### Bug Fixes

* Aztec-nargo curl in the earthfile also
([#10199](#10199))
([985a678](985a678))
* **bb.js:** Don't minify bb.js - webpack config
([#10170](#10170))
([6e7fae7](6e7fae7))
* Docker compose aztec up fix
([#10197](#10197))
([d7ae959](d7ae959))
* Increase test timeouts
([#10205](#10205))
([195aa3d](195aa3d))
* Release l1-contracts
([#10095](#10095))
([29f0d7a](29f0d7a))
* Revert "feat: blobs.
([#9302](#9302))"
([#10187](#10187))
([a415f65](a415f65))


### Miscellaneous

* Added ref to env variables
([#10193](#10193))
([b51fc43](b51fc43))
* **avm:** Operands reordering
([#10182](#10182))
([69bdf4f](69bdf4f)),
closes
[#10136](#10136)
* Fix devbox
([#10201](#10201))
([323eaee](323eaee))
* Misc cleanup
([#10194](#10194))
([dd01417](dd01417))
* Reinstate docs-preview, fix doc publish
([#10213](#10213))
([ed9a0e3](ed9a0e3))
* Replace relative paths to noir-protocol-circuits
([1650446](1650446))
</details>

<details><summary>barretenberg: 0.65.0</summary>

##
[0.65.0](barretenberg-v0.64.0...barretenberg-v0.65.0)
(2024-11-26)


### Features

* **avm:** New public inputs witgen
([#10179](#10179))
([ac8f13e](ac8f13e))
* Origin tags implemented in biggroup
([#10002](#10002))
([c8696b1](c8696b1))
* UltraRollupRecursiveFlavor
([#10088](#10088))
([4418ef2](4418ef2))


### Miscellaneous

* **avm:** Operands reordering
([#10182](#10182))
([69bdf4f](69bdf4f)),
closes
[#10136](#10136)
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
AztecBot added a commit to AztecProtocol/barretenberg that referenced this pull request Nov 27, 2024
@AztecBot
chore(master): Release 0.65.0 (#10181)
ebf3927 
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-package: 0.65.0</summary>

##
[0.65.0](AztecProtocol/aztec-packages@aztec-package-v0.64.0...aztec-package-v0.65.0)
(2024-11-26)


### Features

* **avm:** New public inputs witgen
([#10179](AztecProtocol/aztec-packages#10179))
([ac8f13e](AztecProtocol/aztec-packages@ac8f13e))
</details>

<details><summary>barretenberg.js: 0.65.0</summary>

##
[0.65.0](AztecProtocol/aztec-packages@barretenberg.js-v0.64.0...barretenberg.js-v0.65.0)
(2024-11-26)


### Bug Fixes

* **bb.js:** Don't minify bb.js - webpack config
([#10170](AztecProtocol/aztec-packages#10170))
([6e7fae7](AztecProtocol/aztec-packages@6e7fae7))
</details>

<details><summary>aztec-packages: 0.65.0</summary>

##
[0.65.0](AztecProtocol/aztec-packages@aztec-packages-v0.64.0...aztec-packages-v0.65.0)
(2024-11-26)


### ⚠ BREAKING CHANGES

* remove SharedImmutable
([#10183](AztecProtocol/aztec-packages#10183))
* rename sharedimmutable methods
([#10164](AztecProtocol/aztec-packages#10164))

### Features

* **avm:** New public inputs witgen
([#10179](AztecProtocol/aztec-packages#10179))
([ac8f13e](AztecProtocol/aztec-packages@ac8f13e))
* Blobs.
([#9302](AztecProtocol/aztec-packages#9302))
([03b7e0e](AztecProtocol/aztec-packages@03b7e0e))
* One liner for nodes to join rough-rhino
([#10168](AztecProtocol/aztec-packages#10168))
([3a425e9](AztecProtocol/aztec-packages@3a425e9))
* Origin tags implemented in biggroup
([#10002](AztecProtocol/aztec-packages#10002))
([c8696b1](AztecProtocol/aztec-packages@c8696b1))
* Remove SharedImmutable
([#10183](AztecProtocol/aztec-packages#10183))
([a9f3b5f](AztecProtocol/aztec-packages@a9f3b5f))
* Rename sharedimmutable methods
([#10164](AztecProtocol/aztec-packages#10164))
([ef7cd86](AztecProtocol/aztec-packages@ef7cd86))
* UltraRollupRecursiveFlavor
([#10088](AztecProtocol/aztec-packages#10088))
([4418ef2](AztecProtocol/aztec-packages@4418ef2))


### Bug Fixes

* Aztec-nargo curl in the earthfile also
([#10199](AztecProtocol/aztec-packages#10199))
([985a678](AztecProtocol/aztec-packages@985a678))
* **bb.js:** Don't minify bb.js - webpack config
([#10170](AztecProtocol/aztec-packages#10170))
([6e7fae7](AztecProtocol/aztec-packages@6e7fae7))
* Docker compose aztec up fix
([#10197](AztecProtocol/aztec-packages#10197))
([d7ae959](AztecProtocol/aztec-packages@d7ae959))
* Increase test timeouts
([#10205](AztecProtocol/aztec-packages#10205))
([195aa3d](AztecProtocol/aztec-packages@195aa3d))
* Release l1-contracts
([#10095](AztecProtocol/aztec-packages#10095))
([29f0d7a](AztecProtocol/aztec-packages@29f0d7a))
* Revert "feat: blobs.
([#9302](AztecProtocol/aztec-packages#9302))"
([#10187](AztecProtocol/aztec-packages#10187))
([a415f65](AztecProtocol/aztec-packages@a415f65))


### Miscellaneous

* Added ref to env variables
([#10193](AztecProtocol/aztec-packages#10193))
([b51fc43](AztecProtocol/aztec-packages@b51fc43))
* **avm:** Operands reordering
([#10182](AztecProtocol/aztec-packages#10182))
([69bdf4f](AztecProtocol/aztec-packages@69bdf4f)),
closes
[#10136](AztecProtocol/aztec-packages#10136)
* Fix devbox
([#10201](AztecProtocol/aztec-packages#10201))
([323eaee](AztecProtocol/aztec-packages@323eaee))
* Misc cleanup
([#10194](AztecProtocol/aztec-packages#10194))
([dd01417](AztecProtocol/aztec-packages@dd01417))
* Reinstate docs-preview, fix doc publish
([#10213](AztecProtocol/aztec-packages#10213))
([ed9a0e3](AztecProtocol/aztec-packages@ed9a0e3))
* Replace relative paths to noir-protocol-circuits
([1650446](AztecProtocol/aztec-packages@1650446))
</details>

<details><summary>barretenberg: 0.65.0</summary>

##
[0.65.0](AztecProtocol/aztec-packages@barretenberg-v0.64.0...barretenberg-v0.65.0)
(2024-11-26)


### Features

* **avm:** New public inputs witgen
([#10179](AztecProtocol/aztec-packages#10179))
([ac8f13e](AztecProtocol/aztec-packages@ac8f13e))
* Origin tags implemented in biggroup
([#10002](AztecProtocol/aztec-packages#10002))
([c8696b1](AztecProtocol/aztec-packages@c8696b1))
* UltraRollupRecursiveFlavor
([#10088](AztecProtocol/aztec-packages#10088))
([4418ef2](AztecProtocol/aztec-packages@4418ef2))


### Miscellaneous

* **avm:** Operands reordering
([#10182](AztecProtocol/aztec-packages#10182))
([69bdf4f](AztecProtocol/aztec-packages@69bdf4f)),
closes
[#10136](AztecProtocol/aztec-packages#10136)
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fcarreiro fcarreiro fcarreiro approved these changes

@dbanks12 dbanks12 Awaiting requested review from dbanks12 dbanks12 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some operands reordering in serialization
2 participants
@jeanmon @fcarreiro