From c80329818874cd34cb2ac3eb0355e804cdb462c9 Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Tue, 4 Jun 2024 17:00:03 +0300 Subject: [PATCH 1/2] test: add duplicated stack operands test for the stack operand schedule solver that reproduces #200 issue --- Cargo.lock | 2 ++ codegen/masm/src/codegen/emitter.rs | 5 ++- .../masm/src/codegen/opt/operands/solver.rs | 34 +++++++++++++++++++ tests/integration/Cargo.toml | 2 ++ .../abi_transform/tx_kernel.rs | 11 ++++++ 5 files changed, 53 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 15e361bd3..39c01b10d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2715,9 +2715,11 @@ dependencies = [ "cargo_metadata", "concat-idents", "derive_more", + "env_logger 0.9.3", "expect-test", "filetime", "glob", + "log", "miden-assembly", "miden-core", "miden-diagnostics", diff --git a/codegen/masm/src/codegen/emitter.rs b/codegen/masm/src/codegen/emitter.rs index cb3570d3d..0da2d0c61 100644 --- a/codegen/masm/src/codegen/emitter.rs +++ b/codegen/masm/src/codegen/emitter.rs @@ -205,10 +205,13 @@ impl<'b, 'f: 'b> BlockEmitter<'b, 'f> { let args = self.function.f.dfg.inst_args(inst_info.inst); self.schedule_operands(args, inst_info.plain_arguments()).unwrap_or_else(|err| { panic!( - "failed to schedule operands: {:?} \n for inst:\n {}\n {:?}\n with error: {err:?}", + "failed to schedule operands: {:?} \n for inst: {} {:?}\n with error: {err:?}\n \ + constraints: {:?}\n stack: {:?}", args, inst_info.inst, self.function.f.dfg.inst(inst_info.inst), + inst_info.plain_arguments(), + self.stack, ) }); diff --git a/codegen/masm/src/codegen/opt/operands/solver.rs b/codegen/masm/src/codegen/opt/operands/solver.rs index 17f588066..1a89587c6 100644 --- a/codegen/masm/src/codegen/opt/operands/solver.rs +++ b/codegen/masm/src/codegen/opt/operands/solver.rs @@ -427,6 +427,40 @@ mod tests { } } + /// This test reproduces https://github.com/0xPolygonMiden/compiler/issues/200 + /// where v7 value should be duplicated on the stack + #[test] + fn operand_movement_constraint_solver_duplicate() { + setup(); + let v7 = hir::Value::from_u32(1); + let v16 = hir::Value::from_u32(2); + let v32 = hir::Value::from_u32(3); + let v0 = hir::Value::from_u32(4); + + let tests = [[v32, v7, v16, v0]]; + + for test in tests.into_iter() { + let mut stack = crate::codegen::OperandStack::default(); + for value in test.into_iter().rev() { + stack.push(crate::codegen::TypedValue { + ty: Type::I32, + value, + }); + } + // The v7 is expected to be twice on stack + let expected = [v7, v7, v32, v16]; + let constraints = [Constraint::Copy; 4]; + + match OperandMovementConstraintSolver::new(&expected, &constraints, &stack) { + Ok(solver) => { + let _result = solver.solve().expect("no solution found"); + } + Err(SolverError::AlreadySolved) => panic!("already solved"), + Err(err) => panic!("invalid solver context: {err:?}"), + } + } + } + // Strategy: // // 1. Generate a set of 1..16 operands to form a stack (called `stack`), with no more than 2 diff --git a/tests/integration/Cargo.toml b/tests/integration/Cargo.toml index a343f8580..a6eb78aca 100644 --- a/tests/integration/Cargo.toml +++ b/tests/integration/Cargo.toml @@ -40,3 +40,5 @@ proptest.workspace = true miden-core.workspace = true concat-idents = "1.1" blake3.workspace = true +log.workspace = true +env_logger.workspace = true \ No newline at end of file diff --git a/tests/integration/src/rust_masm_tests/abi_transform/tx_kernel.rs b/tests/integration/src/rust_masm_tests/abi_transform/tx_kernel.rs index 2ad31d939..f8cc3cc2e 100644 --- a/tests/integration/src/rust_masm_tests/abi_transform/tx_kernel.rs +++ b/tests/integration/src/rust_masm_tests/abi_transform/tx_kernel.rs @@ -2,9 +2,20 @@ use expect_test::expect_file; use crate::CompilerTest; +#[allow(unused)] +fn setup_log() { + use log::LevelFilter; + let _ = env_logger::builder() + .filter_level(LevelFilter::Trace) + .format_timestamp(None) + .is_test(true) + .try_init(); +} + #[ignore = "until https://github.com/0xPolygonMiden/compiler/issues/200 is fixed"] #[test] fn test_get_inputs() { + // setup_log(); let main_fn = "() -> Vec {{ get_inputs() }}"; let artifact_name = "abi_transform_tx_kernel_get_inputs"; let mut test = CompilerTest::rust_fn_body_with_sdk(artifact_name, main_fn, true); From 84a4b242adac6bc4b5f0ea6fa39e11c62a2d6616 Mon Sep 17 00:00:00 2001 From: Paul Schoenfelder Date: Mon, 24 Jun 2024 14:34:10 -0400 Subject: [PATCH 2/2] fix: operand solver improperly tracking aliases Due to a bug in how the solver context was initialized, expected outputs with aliases (copies/duplicates) could end up mismatched if the aliases were at the top of the stack. This commit addresses that bug, and also extends the CopyInfo struct so that it can be used for initializing aliases of both the expected outputs and the current operand stack state. Closes #200 --- .../masm/src/codegen/opt/operands/context.rs | 68 ++++++++++++------- .../masm/src/codegen/opt/operands/solver.rs | 8 +-- 2 files changed, 48 insertions(+), 28 deletions(-) diff --git a/codegen/masm/src/codegen/opt/operands/context.rs b/codegen/masm/src/codegen/opt/operands/context.rs index 5883f9838..0110816f5 100644 --- a/codegen/masm/src/codegen/opt/operands/context.rs +++ b/codegen/masm/src/codegen/opt/operands/context.rs @@ -21,8 +21,6 @@ impl SolverContext { constraints: &[Constraint], stack: &crate::codegen::OperandStack, ) -> Result { - use std::collections::btree_map::Entry; - // Compute the expected output on the stack, as well as alias/copy information let mut stack = Stack::from(stack); let mut expected_output = Stack::default(); @@ -35,9 +33,9 @@ impl SolverContext { Constraint::Move => { expected_output.push(value); } - // If we observe a value with copy semantics, then it is - // always referencing an alias, because the original would - // need to be preserved + // If we observe a value with copy semantics, then the expected + // output is always an alias, because the original would need to + // be preserved Constraint::Copy => { expected_output.push(copies.push(value)); } @@ -45,26 +43,13 @@ impl SolverContext { } // Rename multiple occurrences of the same value on the operand stack, if present - let mut renamed = BTreeMap::::default(); + let mut dupes = CopyInfo::default(); for operand in stack.iter_mut().rev() { - match renamed.entry(operand.value) { - Entry::Vacant(entry) => { - entry.insert(0); - } - Entry::Occupied(mut entry) => { - let next_id = entry.get_mut(); - *next_id += 1; - operand.value.set_alias(NonZeroU8::new(*next_id).unwrap()); - } - } + operand.value = dupes.push_if_duplicate(operand.value); } // Determine if the stack is already in the desired order - // - // If copies are required we can't consider the stack in order even if - // the operands we want are in the desired order, because we must make - // copies of them anyway. - let requires_copies = !copies.is_empty(); + let requires_copies = copies.copies_required(); let is_solved = !requires_copies && expected_output.iter().rev().all(|op| &stack[op.pos as usize] == op); if is_solved { @@ -131,24 +116,59 @@ impl CopyInfo { } /// Push a new copy of `value`, returning an alias of that value + /// + /// NOTE: It is expected that `value` is not an alias. pub fn push(&mut self, value: ValueOrAlias) -> ValueOrAlias { use std::collections::btree_map::Entry; + assert!(!value.is_alias()); + self.num_copies += 1; match self.copies.entry(value) { Entry::Vacant(entry) => { - entry.insert(0); + entry.insert(1); value.copy(unsafe { NonZeroU8::new_unchecked(1) }) } Entry::Occupied(mut entry) => { let next_id = entry.get_mut(); *next_id += 1; - value.copy(NonZeroU8::new(*next_id).unwrap()) + value.copy(unsafe { NonZeroU8::new_unchecked(*next_id) }) } } } + /// Push a copy of `value`, but only if `value` has already been seen + /// at least once, i.e. `value` is a duplicate. + /// + /// NOTE: It is expected that `value` is not an alias. + pub fn push_if_duplicate(&mut self, value: ValueOrAlias) -> ValueOrAlias { + use std::collections::btree_map::Entry; + + assert!(!value.is_alias()); + + match self.copies.entry(value) { + // `value` is not a duplicate + Entry::Vacant(entry) => { + entry.insert(0); + value + } + // `value` is a duplicate, record it as such + Entry::Occupied(mut entry) => { + self.num_copies += 1; + let next_id = entry.get_mut(); + *next_id += 1; + value.copy(unsafe { NonZeroU8::new_unchecked(*next_id) }) + } + } + } + + /// Returns true if `value` has at least one copy pub fn has_copies(&self, value: &ValueOrAlias) -> bool { - self.copies.contains_key(value) + self.copies.get(value).map(|count| *count > 0).unwrap_or(false) + } + + /// Returns true if any of the values seen so far have copies + pub fn copies_required(&self) -> bool { + self.copies.values().any(|count| *count > 0) } } diff --git a/codegen/masm/src/codegen/opt/operands/solver.rs b/codegen/masm/src/codegen/opt/operands/solver.rs index 1a89587c6..0304a8a71 100644 --- a/codegen/masm/src/codegen/opt/operands/solver.rs +++ b/codegen/masm/src/codegen/opt/operands/solver.rs @@ -432,10 +432,10 @@ mod tests { #[test] fn operand_movement_constraint_solver_duplicate() { setup(); - let v7 = hir::Value::from_u32(1); - let v16 = hir::Value::from_u32(2); - let v32 = hir::Value::from_u32(3); - let v0 = hir::Value::from_u32(4); + let v7 = hir::Value::from_u32(7); + let v16 = hir::Value::from_u32(16); + let v32 = hir::Value::from_u32(32); + let v0 = hir::Value::from_u32(0); let tests = [[v32, v7, v16, v0]];