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.

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

[2/2] Integration test for basic wallet account code compilation to MASM #186

Merged
merged 3 commits into from
May 24, 2024

Conversation

greenhat
Copy link
Contributor

@greenhat greenhat commented May 13, 2024

This PR is intended to prepare the basic wallet compilation for the presentation for Bali.

@greenhat
Copy link
Contributor Author

The Rust code https://github.com/0xPolygonMiden/compiler/blob/c14c70b076b17ddb083f23f7e5562908537b914c/tests/rust-apps-wasm/rust-sdk/account-test/src/lib.rs#L1-L27

compiles to IR - https://github.com/0xPolygonMiden/compiler/blob/c14c70b076b17ddb083f23f7e5562908537b914c/tests/integration/expected/rust_sdk_account_test/miden_sdk_account_test.hir#L1-L280

But the MASM compilation fails with:

thread 'rust_masm_tests::rust_sdk::account' panicked at codegen/masm/src/codegen/opt/operands/tactics/linear.rs:38:17:
assertion failed: b.value.is_alias()

Commenting the assertion did not get me far.

To reproduce run:

cargo test account

@greenhat
Copy link
Contributor Author

@bitwalker
Copy link
Contributor

@greenhat I've addressed a variety of bugs that were blocking compilation to MASM, but I'm able to compile the account code in the test you put together to MASM.

NOTE: There are a couple of other issues with codegen that need to be addressed, but we can at least put together the code artifacts for a demo for now. To sum up the major items:

  • Currently load/store and side-effecting operations can be reordered around each other, which is incorrect. We can fix this by introducing control dependencies in the treegraph scheduler, which is already implemented, what isn't implemented is assigning control dependencies to the various side-effecting operations, as well as ensuring that certain operations can not be reordered relative to one another (for now, we should preserve the original load/store ordering, and ensure that their ordering with respect to call sites is preserved).
  • Operand stack overflow
  • Read-only data segments
  • Finish up felt representation changes

@greenhat greenhat changed the title [DO NO MERGE] Basic wallet account code compilation to MASM [DO NOT MERGE] Basic wallet account code compilation to MASM May 15, 2024
@greenhat
Copy link
Contributor Author

Do you want to merge this PR, so your fixes got merged, or did you cherry-pick them into your branch(es) already? I can restore the original code for the account test along with this one in this branch.

@bitwalker
Copy link
Contributor

@greenhat You can go ahead and merge this PR, probably easier that way

@greenhat
Copy link
Contributor Author

@greenhat You can go ahead and merge this PR, probably easier that way

I'd rather move your commits either to #182 or even to a separate PR on top of it. Otherwise, they end up in #177 which might take a while to get merged.

@greenhat greenhat force-pushed the greenhat/abi-transform-tests branch from afc1e5e to c9595e8 Compare May 24, 2024 04:08
@greenhat greenhat force-pushed the greenhat/bali-account-comp branch from 44e30d1 to 59db94d Compare May 24, 2024 04:08
@bitwalker bitwalker force-pushed the greenhat/abi-transform-tests branch from c9595e8 to afc1e5e Compare May 24, 2024 04:46
@greenhat greenhat force-pushed the greenhat/bali-account-comp branch from 59db94d to 466680d Compare May 24, 2024 12:51
@greenhat greenhat force-pushed the greenhat/abi-transform-tests branch from afc1e5e to 2036f9a Compare May 24, 2024 12:51
@greenhat greenhat changed the title [DO NOT MERGE] Basic wallet account code compilation to MASM [2/2] Integration test for basic wallet account code compilation to MASM May 24, 2024
@greenhat greenhat force-pushed the greenhat/bali-account-comp branch from 466680d to b54fe6d Compare May 24, 2024 14:59
@greenhat greenhat marked this pull request as ready for review May 24, 2024 15:00
@greenhat greenhat requested a review from bitwalker May 24, 2024 15:34
Base automatically changed from greenhat/abi-transform-tests to main May 24, 2024 17:14
@bitwalker bitwalker merged commit 3693ae1 into main May 24, 2024
4 checks passed
@bitwalker bitwalker deleted the greenhat/bali-account-comp branch May 24, 2024 17:16
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