From fc2a90b78eb72d97e19100c93ca80c9a2892563c Mon Sep 17 00:00:00 2001 From: Vaivaswatha N Date: Fri, 28 Jun 2024 22:13:14 +0530 Subject: [PATCH] Add a simple simplify-cfg optimization for assembly (#6197) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description When compiling code such as ``` fn main() { while true {} } ``` we would end up with unreachable code in the assembly: ``` .program: .2 ; --- start of function: main_0 --- pusha .2 ; save all regs move $$locbase $sp ; save locals base register for main_0 cfei i0 ; allocate 0 bytes for locals and 0 slots for call arguments. move $r28 $$reta ; save reta move $r29 $$retv ; save retv .6 ji .7 .7 ji .7 .3 cfsi i0 ; free 0 bytes for locals and 0 slots for extra call arguments. move $$reta $r28 ; restore reta popa .2 ; restore all regs jmp $$reta ; return from call ;; --- Data --- .data: ``` Here the label `.3` and what follows is unreachable because the program gets stuck in an infinite loops with label `.7`. This means that the instruction `move $r28 $$reta` is dead and hence removed by ASM DCE. Our [verifier](https://github.com/FuelLabs/sway/blob/4a6cf38143b398a568d9587f79bfa46c15f48459/sway-core/src/asm_generation/fuel/abstract_instruction_set.rs#L131) on the other hand will see a use of `$r28` in the unreachable code. But with the definition having been eliminated, it will now flag an error. The verifier is unaware of reachability. Making the verifier more accurate is probably an overkill. So we workaround this by adding an unreachable code eliminator optimization in the assembly stage. While it has its own benefits (of removing unreachable code), it'll also make the verifier happy as-is. The algorithm is linear in the size of the code. Closes #6174. --------- Co-authored-by: Igor Rončević --- .../fuel/abstract_instruction_set.rs | 1 + .../src/asm_generation/fuel/optimizations.rs | 44 ++++++++++++++++++- .../abort_control_flow_good/test.toml | 2 +- .../break_and_continue_block_ret/test.toml | 2 +- 4 files changed, 45 insertions(+), 4 deletions(-) diff --git a/sway-core/src/asm_generation/fuel/abstract_instruction_set.rs b/sway-core/src/asm_generation/fuel/abstract_instruction_set.rs index cd1156f38e8..799773ab219 100644 --- a/sway-core/src/asm_generation/fuel/abstract_instruction_set.rs +++ b/sway-core/src/asm_generation/fuel/abstract_instruction_set.rs @@ -27,6 +27,7 @@ impl AbstractInstructionSet { pub(crate) fn optimize(self, data_section: &DataSection) -> AbstractInstructionSet { self.const_indexing_aggregates_function(data_section) .dce() + .simplify_cfg() .remove_sequential_jumps() .remove_redundant_moves() .remove_unused_ops() diff --git a/sway-core/src/asm_generation/fuel/optimizations.rs b/sway-core/src/asm_generation/fuel/optimizations.rs index 63fd044c039..4800ba6906f 100644 --- a/sway-core/src/asm_generation/fuel/optimizations.rs +++ b/sway-core/src/asm_generation/fuel/optimizations.rs @@ -1,11 +1,11 @@ -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashMap}; use either::Either; use rustc_hash::{FxHashMap, FxHashSet}; use crate::{ asm_generation::fuel::compiler_constants, - asm_lang::{ControlFlowOp, VirtualImmediate12, VirtualOp, VirtualRegister}, + asm_lang::{ControlFlowOp, Label, VirtualImmediate12, VirtualOp, VirtualRegister}, }; use super::{ @@ -269,4 +269,44 @@ impl AbstractInstructionSet { self } + + // Remove unreachable instructions. + pub(crate) fn simplify_cfg(mut self) -> AbstractInstructionSet { + let ops = &self.ops; + + if ops.is_empty() { + return self; + } + + // Keep track of a map between jump labels and op indices. Useful to compute op successors. + let mut label_to_index: HashMap = HashMap::default(); + for (idx, op) in ops.iter().enumerate() { + if let Either::Right(ControlFlowOp::Label(op_label)) = op.opcode { + label_to_index.insert(op_label, idx); + } + } + + let mut reachables = vec![false; ops.len()]; + let mut worklist = vec![0]; + while let Some(op_idx) = worklist.pop() { + assert!(!reachables[op_idx]); + reachables[op_idx] = true; + let op = &ops[op_idx]; + for s in &op.successors(op_idx, ops, &label_to_index) { + if !reachables[*s] { + worklist.push(*s); + } + } + } + + let reachable_ops = self + .ops + .into_iter() + .enumerate() + .filter_map(|(idx, op)| if reachables[idx] { Some(op) } else { None }) + .collect(); + self.ops = reachable_ops; + + self + } } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/abort_control_flow_good/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/language/abort_control_flow_good/test.toml index dd52027cc51..6fdbfa9c1ef 100644 --- a/test/src/e2e_vm_tests/test_programs/should_pass/language/abort_control_flow_good/test.toml +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/abort_control_flow_good/test.toml @@ -1,4 +1,4 @@ -category = "disabled" # TODO: Enable once https://github.com/FuelLabs/sway/issues/6174 is fixed. +category = "run" expected_result = { action = "revert", value = 42 } expected_result_new_encoding = { action = "revert", value = 42 } validate_abi = true diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/break_and_continue_block_ret/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/language/break_and_continue_block_ret/test.toml index 4a63591381a..b8f355a8aa9 100644 --- a/test/src/e2e_vm_tests/test_programs/should_pass/language/break_and_continue_block_ret/test.toml +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/break_and_continue_block_ret/test.toml @@ -1,3 +1,3 @@ -category = "disabled" # TODO: Enable once https://github.com/FuelLabs/sway/issues/6174 is fixed. +category = "compile" # not: $()This returns a value of type unknown, which is not assigned to anything and is ignored.