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

[1/2] stdlib blake3 call MASM compilation test #177

Merged
merged 4 commits into from
May 24, 2024

Conversation

greenhat
Copy link
Contributor

@greenhat greenhat commented Apr 30, 2024

This PR adds a compilation test for calling the blake3 hash function from the stdlib.

Base automatically changed from greenhat/i154-intrinsics-tests to main May 1, 2024 04:10
@greenhat greenhat changed the title [8/x] Miden ABI transformation tests Miden ABI transformation tests May 2, 2024
@greenhat greenhat force-pushed the greenhat/abi-transform-tests branch from f417f35 to c5919f4 Compare May 3, 2024 13:30
@greenhat greenhat changed the base branch from main to greenhat/driver-support-ir-component May 3, 2024 13:31
@greenhat greenhat changed the title Miden ABI transformation tests [2/x] Miden ABI transformation tests May 3, 2024
@greenhat greenhat force-pushed the greenhat/driver-support-ir-component branch from 31f9986 to 139fad4 Compare May 4, 2024 13:33
@greenhat greenhat force-pushed the greenhat/abi-transform-tests branch from 38bfc78 to 0b50c12 Compare May 4, 2024 13:34
@greenhat greenhat force-pushed the greenhat/abi-transform-tests branch from 0b50c12 to 820cd76 Compare May 6, 2024 12:33
@greenhat greenhat force-pushed the greenhat/driver-support-ir-component branch 2 times, most recently from b5a26a7 to 5c7f9ae Compare May 7, 2024 11:53
@greenhat greenhat force-pushed the greenhat/abi-transform-tests branch 2 times, most recently from 2b83db9 to 100cf05 Compare May 7, 2024 12:18
@greenhat greenhat force-pushed the greenhat/driver-support-ir-component branch from 5c7f9ae to 0f544c1 Compare May 7, 2024 12:18
Base automatically changed from greenhat/driver-support-ir-component to main May 7, 2024 12:33
@greenhat greenhat changed the title [2/x] Miden ABI transformation tests Miden ABI transformation tests May 7, 2024
@greenhat greenhat force-pushed the greenhat/abi-transform-tests branch 2 times, most recently from a41bc0a to afc1e5e Compare May 7, 2024 12:41
@greenhat greenhat changed the base branch from main to bitwalker/emit-stores May 7, 2024 12:41
@greenhat
Copy link
Contributor Author

greenhat commented May 7, 2024

@bitwalker I rebased this PR on top of #182, and now I hit an assertion in emit_br

assert_eq!(self.controlling_loop, None);

Here is the backtrace:

thread 'rust_masm_tests::abi_transform::stdlib::test_blake3_hash' panicked at codegen/masm/src/codegen/emitter.rs:1006:17:
assertion `left == right` failed
  left: Some(loop0)
 right: None
stack backtrace:
   0: rust_begin_unwind
             at /rustc/2d24fe591f30386d6d5fc2bb941c78d7266bf10f/library/std/src/panicking.rs:645:5
   1: core::panicking::panic_fmt
             at /rustc/2d24fe591f30386d6d5fc2bb941c78d7266bf10f/library/core/src/panicking.rs:72:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
             at /rustc/2d24fe591f30386d6d5fc2bb941c78d7266bf10f/library/core/src/panicking.rs:298:5
   4: miden_codegen_masm::codegen::emitter::BlockEmitter::target_controlling_loop
             at /Users/dzadorozhnyi/src/miden-ir/codegen/masm/src/codegen/emitter.rs:1006:17
   5: miden_codegen_masm::codegen::emitter::BlockEmitter::emit_br
             at /Users/dzadorozhnyi/src/miden-ir/codegen/masm/src/codegen/emitter.rs:291:36
   6: miden_codegen_masm::codegen::emitter::BlockEmitter::emit_inst
             at /Users/dzadorozhnyi/src/miden-ir/codegen/masm/src/codegen/emitter.rs:212:40
   7: miden_codegen_masm::codegen::emitter::BlockEmitter::emit
             at /Users/dzadorozhnyi/src/miden-ir/codegen/masm/src/codegen/emitter.rs:185:48
   8: miden_codegen_masm::codegen::emitter::FunctionEmitter::emit
             at /Users/dzadorozhnyi/src/miden-ir/codegen/masm/src/codegen/emitter.rs:136:21
   9: <miden_codegen_masm::convert::ConvertHirToMasm<&miden_hir::function::Function> as miden_hir::pass::conversion::ConversionPass>::convert
             at /Users/dzadorozhnyi/src/miden-ir/codegen/masm/src/convert.rs:188:13
  10: <miden_codegen_masm::convert::ConvertHirToMasm<miden_hir::module::Module> as miden_hir::pass::conversion::ConversionPass>::convert
             at /Users/dzadorozhnyi/src/miden-ir/codegen/masm/src/convert.rs:136:33
  11: <midenc_compile::stages::codegen::CodegenStage as midenc_compile::stage::Stage>::run
             at /Users/dzadorozhnyi/src/miden-ir/midenc-compile/src/stages/codegen.rs:36:39
  12: <midenc_compile::stage::Chain<A,B> as midenc_compile::stage::Stage>::run
             at /Users/dzadorozhnyi/src/miden-ir/midenc-compile/src/stage.rs:81:9
  13: midenc_compile::compile_inputs
             at /Users/dzadorozhnyi/src/miden-ir/midenc-compile/src/lib.rs:191:5
  14: midenc_compile::compile_to_memory
             at /Users/dzadorozhnyi/src/miden-ir/midenc-compile/src/lib.rs:170:11
  15: miden_integration_tests::compiler_test::CompilerTest::compile_wasm_to_masm_program
             at ./src/compiler_test.rs:527:15
  16: miden_integration_tests::compiler_test::CompilerTest::ir_masm_program
             at ./src/compiler_test.rs:511:27
  17: miden_integration_tests::compiler_test::CompilerTest::expect_masm
             at ./src/compiler_test.rs:476:23
  18: miden_integration_tests::rust_masm_tests::abi_transform::stdlib::test_blake3_hash
             at ./src/rust_masm_tests/abi_transform/stdlib.rs:21:5
  19: miden_integration_tests::rust_masm_tests::abi_transform::stdlib::test_blake3_hash::{{closure}}
             at ./src/rust_masm_tests/abi_transform/stdlib.rs:12:22
  20: core::ops::function::FnOnce::call_once
             at /rustc/2d24fe591f30386d6d5fc2bb941c78d7266bf10f/library/core/src/ops/function.rs:250:5
  21: core::ops::function::FnOnce::call_once
             at /rustc/2d24fe591f30386d6d5fc2bb941c78d7266bf10f/library/core/src/ops/function.rs:250:5

The IR is https://github.com/0xPolygonMiden/compiler/blob/afc1e5ea1438984ef336e6f74216c6764a07934b/tests/integration/expected/abi_transform_stdlib_blake3_hash.hir

@bitwalker
Copy link
Contributor

Do you have some repro steps I can follow to replicate locally? I'll need to poke at it a bit. Definitely odd that a controlling loop is set, but the transition (current_block -> target_block) is going from a block that ostensibly doesn't belong to a loop, to one that does - in that scenario, there should be no controlling loop (otherwise current_block would belong to it), but there are a few edge cases due to how the controlling loop state is managed, and either the assertion is overly restrictive, or a more fine-grained condition needs to be checked.

You can try disabling the assertion temporarily to see how far that gets you while I investigate, but that will only help you if the assertion is overly restrictive.

@greenhat
Copy link
Contributor Author

greenhat commented May 7, 2024

Do you have some repro steps I can follow to replicate locally? I'll need to poke at it a bit. Definitely odd that a controlling loop is set, but the transition (current_block -> target_block) is going from a block that ostensibly doesn't belong to a loop, to one that does - in that scenario, there should be no controlling loop (otherwise current_block would belong to it), but there are a few edge cases due to how the controlling loop state is managed, and either the assertion is overly restrictive, or a more fine-grained condition needs to be checked.

cargo test blake3 will run the test that causes the issue.

You can try disabling the assertion temporarily to see how far that gets you while I investigate, but that will only help you if the assertion is overly restrictive.

After disabling the assertion, I hit the addressable stack limit(16) with index 16.

@bitwalker
Copy link
Contributor

After disabling the assertion, I hit the addressable stack limit(16) with index 16.

Awesome 🤦, I was hoping we'd have a bit more time before hitting that wall, but I guess at least now we have an easily reproducible real-world program that hits it. I'll add this to my TODO list

@greenhat greenhat force-pushed the greenhat/abi-transform-tests branch from afc1e5e to c9595e8 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
Base automatically changed from bitwalker/emit-stores to main May 24, 2024 12:41
@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 Miden ABI transformation tests blake3 MASM compilation test May 24, 2024
@greenhat greenhat changed the title blake3 MASM compilation test stdlib blake3 call MASM compilation test May 24, 2024
@greenhat greenhat changed the title stdlib blake3 call MASM compilation test [1/2] stdlib blake3 call MASM compilation test May 24, 2024
@greenhat greenhat force-pushed the greenhat/abi-transform-tests branch from 6f42ceb to 95f268a Compare May 24, 2024 14:59
@greenhat greenhat marked this pull request as ready for review May 24, 2024 15:01
@greenhat greenhat requested a review from bitwalker May 24, 2024 15:22
@bitwalker bitwalker merged commit 6acf836 into main May 24, 2024
4 checks passed
@bitwalker bitwalker deleted the greenhat/abi-transform-tests branch May 24, 2024 17:14
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