Reorder block creation for while loops. (FuelLabs#3619)

Unfortunately the register allocator is a little bit fragile and requires all register uses are listed after their definitions, regardless of topological order. When compiling `while` loops we have put the 'end' block before the body which could cause these sort of out of order definitions. This change ensures `while` bodies are between the conditional and end blocks. Co-authored-by: Vaivaswatha Nagaraj <[email protected]>
jackal246 · Dec 17, 2022 · c511ccd · c511ccd
1 parent dc06955
commit c511ccd
Show file tree

Hide file tree

Showing 11 changed files with 139 additions and 88 deletions.
diff --git a/sway-core/src/asm_generation/abstract_instruction_set.rs b/sway-core/src/asm_generation/abstract_instruction_set.rs
@@ -30,7 +30,7 @@ impl AbstractInstructionSet {
             .remove_unused_ops()
     }
 
-    /// Removes any jumps that jump to the subsequent line
+    /// Removes any jumps to the subsequent line.
     fn remove_sequential_jumps(mut self) -> AbstractInstructionSet {
         let dead_jumps: Vec<_> = self
             .ops
@@ -145,8 +145,16 @@ impl AbstractInstructionSet {
         if def_regs.is_superset(&use_regs) {
             Ok(self)
         } else {
-            Err(CompileError::Internal(
-                "Program erroneously uses uninitialized virtual registers.",
+            let bad_regs = use_regs
+                .difference(&def_regs)
+                .map(|reg| match reg {
+                    VirtualRegister::Virtual(name) => format!("$r{name}"),
+                    VirtualRegister::Constant(creg) => creg.to_string(),
+                })
+                .collect::<Vec<_>>()
+                .join(", ");
+            Err(CompileError::InternalOwned(
+                format!("Program erroneously uses uninitialized virtual registers: {bad_regs}"),
                 Span::dummy(),
             ))
         }

diff --git a/sway-core/src/ir_generation/function.rs b/sway-core/src/ir_generation/function.rs
@@ -1461,23 +1461,25 @@ impl<'te> FnCompiler<'te> {
         // We're dancing around a bit here to make the blocks sit in the right order.  Ideally we
         // have the cond block, followed by the body block which may contain other blocks, and the
         // final block comes after any body block(s).
+        //
+        // NOTE: This is currently very important!  There is a limitation in the register allocator
+        // which requires that all value uses are after the value definitions, where 'after' means
+        // later in the list of instructions, as opposed to in the control flow sense.
+        //
+        // Hence the need for a 'break' block which does nothing more than jump to the final block,
+        // as we need to construct the final block after the body block, but we need somewhere to
+        // break to during the body block construction.
 
         // Jump to the while cond block.
         let cond_block = self.function.create_block(context, Some("while".into()));
-
         if !self.current_block.is_terminated(context) {
             self.current_block.ins(context).branch(cond_block, vec![]);
         }
 
-        // Fill in the body block now, jump unconditionally to the cond block at its end.
-        let body_block = self
-            .function
-            .create_block(context, Some("while_body".into()));
-
-        // Create the final block after we're finished with the body.
-        let final_block = self
+        // Create the break block.
+        let break_block = self
             .function
-            .create_block(context, Some("end_while".into()));
+            .create_block(context, Some("while_break".into()));
 
         // Keep track of the previous blocks we have to jump to in case of a break or a continue.
         // This should be `None` if we're not in a loop already or the previous break or continue
@@ -1486,11 +1488,13 @@ impl<'te> FnCompiler<'te> {
         let prev_block_to_continue_to = self.block_to_continue_to;
 
         // Keep track of the current blocks to jump to in case of a break or continue.
-        self.block_to_break_to = Some(final_block);
+        self.block_to_break_to = Some(break_block);
         self.block_to_continue_to = Some(cond_block);
 
-        // Compile the body and a branch to the condition block if no branch is already present in
-        // the body block
+        // Fill in the body block now, jump unconditionally to the cond block at its end.
+        let body_block = self
+            .function
+            .create_block(context, Some("while_body".into()));
         self.current_block = body_block;
         self.compile_code_block(context, md_mgr, body)?;
         if !self.current_block.is_terminated(context) {
@@ -1501,7 +1505,16 @@ impl<'te> FnCompiler<'te> {
         self.block_to_break_to = prev_block_to_break_to;
         self.block_to_continue_to = prev_block_to_continue_to;
 
-        // Add the conditional which jumps into the body or out to the final block.
+        // Create the final block now we're finished with the body.
+        let final_block = self
+            .function
+            .create_block(context, Some("end_while".into()));
+
+        // Add an unconditional jump from the break block to the final block.
+        break_block.ins(context).branch(final_block, vec![]);
+
+        // Add the conditional in the cond block which jumps into the body or out to the final
+        // block.
         self.current_block = cond_block;
         let cond_value = self.compile_expression(context, md_mgr, condition)?;
         if !self.current_block.is_terminated(context) {

diff --git a/sway-ir/src/error.rs b/sway-ir/src/error.rs
@@ -45,6 +45,7 @@ pub enum IrError {
     VerifyIntToPtrToCopyType(String),
     VerifyIntToPtrUnknownSourceType,
     VerifyLoadFromNonPointer,
+    VerifyMemcopyNonExistentPointer,
     VerifyMismatchedReturnTypes(String),
     VerifyBlockArgMalformed,
     VerifyPtrCastFromNonPointer,
@@ -253,6 +254,12 @@ impl fmt::Display for IrError {
             IrError::VerifyLoadFromNonPointer => {
                 write!(f, "Verification failed: Load must be from a pointer.")
             }
+            IrError::VerifyMemcopyNonExistentPointer => {
+                write!(
+                    f,
+                    "Verification failed: Attempt to use non pointer with `mem_copy`."
+                )
+            }
             IrError::VerifyMismatchedReturnTypes(fn_str) => write!(
                 f,
                 "Verification failed: Function {fn_str} return type must match its RET \

diff --git a/sway-ir/src/optimize/inline.rs b/sway-ir/src/optimize/inline.rs
@@ -180,7 +180,7 @@ pub fn inline_function_call(
         context,
         call_site,
         post_block.get_arg(context, 0).unwrap(),
-        Some(post_block),
+        None,
     );
 
     // Take the locals from the inlined function and add them to this function.  `value_map` is a

diff --git a/sway-ir/src/verify.rs b/sway-ir/src/verify.rs
@@ -652,7 +652,7 @@ impl<'a> InstructionVerifier<'a> {
 
     fn verify_mem_copy(
         &self,
-        _dst_val: &Value,
+        dst_val: &Value,
         _src_val: &Value,
         _byte_len: &u64,
     ) -> Result<(), IrError> {
@@ -662,18 +662,17 @@ impl<'a> InstructionVerifier<'a> {
         //| XXX Pointers are broken, pending https://github.com/FuelLabs/sway/issues/2819
         //| So here we may still get non-pointers, but still ref-types, passed as the source for
         //| mem_copy, especially when dealing with constant b256s or similar.
-        //|if !dst_val.get_pointer(self.context).is_some()
+        if dst_val.get_pointer(self.context).is_none()
         //|    || !(src_val.get_pointer(self.context).is_some()
         //|        || matches!(
         //|            src_val.get_instruction(self.context),
         //|            Some(Instruction::GetStorageKey) | Some(Instruction::IntToPtr(..))
         //|        ))
-        //|{
-        //|    Err(IrError::VerifyGetNonExistentPointer)
-        //|} else {
-        //|    Ok(())
-        //|}
-        Ok(())
+        {
+            Err(IrError::VerifyMemcopyNonExistentPointer)
+        } else {
+            Ok(())
+        }
     }
 
     fn verify_ret(&self, val: &Value, ty: &Type) -> Result<(), IrError> {

diff --git a/test/src/e2e_vm_tests/mod.rs b/test/src/e2e_vm_tests/mod.rs
@@ -241,17 +241,24 @@ impl TestContext {
                     output.push_str(&out);
                     contract_ids.push(result);
                 }
-                let contract_ids: Result<Vec<ContractId>, anyhow::Error> =
-                    contract_ids.into_iter().collect();
+                let contract_ids = contract_ids.into_iter().collect::<Result<Vec<_>, _>>()?;
                 let (result, out) =
-                    harness::runs_on_node(&name, &context.run_config, &contract_ids?).await;
+                    harness::runs_on_node(&name, &context.run_config, &contract_ids).await;
                 output.push_str(&out);
 
                 let receipt = result?;
-                assert!(receipt.iter().all(|res| !matches!(
-                    res,
-                    fuel_tx::Receipt::Revert { .. } | fuel_tx::Receipt::Panic { .. }
-                )));
+                if !receipt.iter().all(|res| {
+                    !matches!(
+                        res,
+                        fuel_tx::Receipt::Revert { .. } | fuel_tx::Receipt::Panic { .. }
+                    )
+                }) {
+                    println!();
+                    for cid in contract_ids {
+                        println!("Deployed contract: 0x{cid}");
+                    }
+                    panic!("Receipts contain reverts or panics: {receipt:?}");
+                }
                 assert!(receipt.len() >= 2);
                 assert_matches!(receipt[receipt.len() - 2], fuel_tx::Receipt::Return { .. });
                 assert_eq!(receipt[receipt.len() - 2].val().unwrap(), val);

diff --git a/...ests/test_programs/should_pass/require_contract_deployment/call_basic_storage/src/main.sw b/...ests/test_programs/should_pass/require_contract_deployment/call_basic_storage/src/main.sw
@@ -3,7 +3,7 @@ use basic_storage_abi::{Quad, StoreU64};
 use std::assert::assert;
 
 fn main() -> u64 {
-    let addr = abi(StoreU64, 0x3b4a1dcf91b22eb138d14dd4aab4b0727f9c6fa855617860e59b8ba7d0fc8a07);
+    let addr = abi(StoreU64, 0x8384cd87cb862d1c4e16943c6cd7c72892ce81e0ead639fd7b71bb45236d7228);
     let key = 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
     let value = 4242;
 

diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/test_abis/basic_storage_abi/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/test_abis/basic_storage_abi/Forc.lock
@@ -1,9 +1,8 @@
 [[package]]
 name = 'basic_storage_abi'
-source = 'root'
+source = 'member'
 dependencies = ['core']
 
 [[package]]
 name = 'core'
 source = 'path+from-root-3777C332FD2E3D7A'
-dependencies = []
diff --git a/test/src/ir_generation/tests/break.sw b/test/src/ir_generation/tests/break.sw
@@ -12,40 +12,49 @@ fn main() {
     }
 }
 
-// check: br $(while=$ID)()
+// We currently use a 'while_break' block to help enforce linear order, where value definitions are
+// always before their uses, when read top to bottom.  Required only due to a limitation in the
+// register allocator which in turn will be fixed in the short term.
 
-// OUTER LOOP: while / while_body / end_while
-// Jump to first inner loop in body, `break` forces return when done rather than jump back to `while`.
+// check: br $(outer_while_cond=$ID)()
 
-// check: $while():
-// check: cbr $VAL, $(while_body=$ID)(), $(end_while=$ID)()
+// OUTER WHILE LOOP
+// check: $outer_while_cond():
+// check: cbr $VAL, $(outer_while_body=$ID)(), $(outer_while_end=$ID)()
 
-// check: $while_body():
-// check: br $(while0=$ID)()
+// check: $(outer_while_break=$ID)():
+// check: br $outer_while_end()
 
-// check: $end_while():
-// check: ret () $VAL
+// check: $outer_while_body():
+// check: br $(inner1_while_cond=$ID)()
+
+
+// FIRST INNER LOOP
+// check: $inner1_while_cond():
+// check: cbr $VAL, $(inner1_while_body=$ID)(), $(inner1_while_end=$ID)()
 
-// FIRST INNER LOOP: while0 / while_body1 / end_while2
-// `break` forces jump to `end_while2` in body, jumps to second inner loop when done.
+// check: $(inner1_while_break=$ID)():
+// check: br $inner1_while_end()
 
-// check: $while0():
-// check: cbr $VAL, $(while_body1=$ID)(), $(end_while2=$ID)()
+// check: $inner1_while_body():
+// check: br $inner1_while_break()
 
-// check: $while_body1():
-// check: br $end_while2()
+// check: $inner1_while_end():
+// check: br $(inner2_while_cond=$ID)()
 
-// check: $end_while2():
-// check: br $(while3=$ID)()
 
-// SECOND INNER LOOP: while3 / while_body4 / end_while5
-// `break` forces jump to `end_while5` in body, jumps to outer loop when done.
+// SECOND INNER LOOP
+// check: $inner2_while_cond():
+// check: cbr $VAL, $(inner2_while_body=$ID)(), $(inner2_while_end=$ID)()
 
-// check: $while3():
-// check: cbr $VAL, $(while_body4=$ID)(), $(end_while5=$ID)()
+// check: $(inner2_while_break=$ID)():
+// check: br $inner2_while_end()
 
-// check: $while_body4():
-// check: br $end_while5()
+// check: $inner2_while_body():
+// check: br $inner2_while_break()
 
-// check: $end_while5():
-// check: br $end_while()
+// check: $inner2_while_end():
+// check: br $outer_while_break()
+
+// check: $outer_while_end():
+// check: ret () $VAL
diff --git a/test/src/ir_generation/tests/continue.sw b/test/src/ir_generation/tests/continue.sw
@@ -13,40 +13,46 @@ fn main() {
 }
 
 
-// check: br $(while=$ID)()
+// OUTER LOOP
+// check: br $(outer_while_cond=$ID)()
 
-// OUTER LOOP: while / while_body / end_while
-// Jump to first inner loop in body, return when done.
+// check: $outer_while_cond():
+// check: cbr $VAL, $(outer_while_body=$ID)(), $(outer_while_end=$ID)()
 
-// check: $while():
-// check: cbr $VAL, $(while_body=$ID)(), $(end_while=$ID)()
+// check: $(outer_while_break=$ID)():
+// check: br $outer_while_end()
 
-// check: $while_body():
-// check: br $(while0=$ID)
+// check: $outer_while_body():
+// check: br $(inner1_while_cond=$ID)()
 
-// check: $end_while():
-// check: ret () $VAL
 
-// FIRST INNER LOOP: while0 / while_body1 / end_while2
-// `continue` forces jump to `while0` in body, branch to second inner loop when done.
+// FIRST INNER LOOP
+// check: $inner1_while_cond():
+// check: cbr $VAL, $(inner1_while_body=$ID)(), $(inner1_while_end=$ID)()
+
+// check: $(inner1_while_break=$ID)():
+// check: br $inner1_while_end()
+
+// check: $inner1_while_body():
+// check: br $inner1_while_cond()
 
-// check: $while0():
-// check: cbr $VAL, $(while_body1=$ID)(), $(end_while2=$ID)()
+// check: $inner1_while_end():
+// check: br $(inner2_while_cond=$ID)()
 
-// check: $while_body1():
-// check: br $while0()
 
-// check: $end_while2():
-// check: br $(while3=$ID)
+// SECOND INNER LOOP
+// check: $inner2_while_cond():
+// check: cbr $VAL, $(inner_while_body=$ID)(), $(inner2_while_end=$ID)()
 
-// SECOND INNER LOOP: while3 / while_body4 / end_while5
-// `continue` forces jump to `while3` in body, branch to outer loop when done.
+// check: $(inner2_while_break=$ID)():
+// check: br $inner2_while_end()
 
-// check: $while3():
-// check: cbr $VAL, $(while_body4=$ID)(), $(end_while5=$ID)()
+// check: $inner_while_body():
+// check: br $inner2_while_cond()
 
-// check: $while_body4():
-// check: br $while3()
+// check: $inner2_while_end():
+// check: br $outer_while_cond()
 
-// check: $end_while5():
-// check: br $while()
+
+// check: $outer_while_end():
+// check: ret () $VAL
diff --git a/test/src/ir_generation/tests/let_reassign_while_loop.sw b/test/src/ir_generation/tests/let_reassign_while_loop.sw
@@ -13,13 +13,16 @@ fn main() -> bool {
 // check: $while():
 // check: cbr $VAL, $(while_body=$ID)(), $(end_while=$ID)()
 
+// check: $(while_break=$ID)():
+// check: br $end_while()
+
 // check: $while_body():
 // check: cbr $VAL, $(block0=$ID)(), $(block1=$ID)($VAL)
 
-// check: $end_while():
-
 // check: $block0():
-// check: br $(block1=$ID)
+// check: br $(block1=$ID)($VAL)
 
 // check: $block1($VAL: bool):
 // check: br $while
+
+// check: $end_while():