Don't force inliner in case of ref type function args (FuelLabs#4823)

This was required previously, but after the IR refactoring (FuelLabs#4336), this is no longer required, and actually worsens things. Fixes FuelLabs#4747. For the two tests mentioned there, (both based on https://github.com/hashcloak/fuel-crypto/) the compile time now comes down from 1. 30m -> 16s 2. 180m -> 31s Also fixes two bugs uncovered: 1. Bug in DCE - Fixes FuelLabs#4763. (FuelLabs@1b512b8) 2. Bug in interaction b/w asm gen and reg alloc spiller (FuelLabs@7a40496). --------- Co-authored-by: IGI-111 <[email protected]>
rich-mind · Jul 21, 2023 · 5318edd · 5318edd
1 parent 303a7a3
commit 5318edd
Show file tree

Hide file tree

Showing 16 changed files with 101 additions and 40 deletions.
diff --git a/sway-core/src/asm_generation/fuel/functions.rs b/sway-core/src/asm_generation/fuel/functions.rs
@@ -5,8 +5,9 @@ use crate::{
         ProgramKind,
     },
     asm_lang::{
-        virtual_register::*, Op, OrganizationalOp, VirtualImmediate12, VirtualImmediate18,
-        VirtualImmediate24, VirtualOp,
+        virtual_register::{self, *},
+        Op, OrganizationalOp, VirtualImmediate12, VirtualImmediate18, VirtualImmediate24,
+        VirtualOp,
     },
     decl_engine::DeclRef,
     error::*,
@@ -20,6 +21,8 @@ use either::Either;
 use sway_error::error::CompileError;
 use sway_types::Ident;
 
+use super::data_section::DataId;
+
 /// A summary of the adopted calling convention:
 ///
 /// - Function arguments are passed left to right in the reserved registers.  Extra args are passed
@@ -194,6 +197,8 @@ impl<'ir, 'eng> FuelAsmBuilder<'ir, 'eng> {
         let mut warnings = Vec::new();
         let mut errors = Vec::new();
 
+        let locals_alloc_result = self.alloc_locals(function);
+
         if func_is_entry {
             let result = Into::<CompileResult<()>>::into(self.compile_external_args(function));
             check!(result, return err(warnings, errors), warnings, errors);
@@ -223,7 +228,7 @@ impl<'ir, 'eng> FuelAsmBuilder<'ir, 'eng> {
             self.return_ctxs.push((end_label, retv));
         }
 
-        self.init_locals(function);
+        self.init_locals(locals_alloc_result);
 
         // Compile instructions. Traverse the IR blocks in reverse post order. This guarantees that
         // each block is processed after all its CFG predecessors have been processed.
@@ -608,7 +613,14 @@ impl<'ir, 'eng> FuelAsmBuilder<'ir, 'eng> {
             .push(Op::unowned_jump_label(success_label));
     }
 
-    fn init_locals(&mut self, function: Function) {
+    fn alloc_locals(
+        &mut self,
+        function: Function,
+    ) -> (
+        u64,
+        virtual_register::VirtualRegister,
+        Vec<(u64, u64, DataId)>,
+    ) {
         // If they're immutable and have a constant initialiser then they go in the data section.
         //
         // Otherwise they go in runtime allocated space, either a register or on the stack.
@@ -681,7 +693,17 @@ impl<'ir, 'eng> FuelAsmBuilder<'ir, 'eng> {
             comment: format!("allocate {locals_size} bytes for locals"),
             owning_span: None,
         });
+        (locals_size, locals_base_reg, init_mut_vars)
+    }
 
+    fn init_locals(
+        &mut self,
+        (locals_size, locals_base_reg, init_mut_vars): (
+            u64,
+            virtual_register::VirtualRegister,
+            Vec<(u64, u64, DataId)>,
+        ),
+    ) {
         // Initialise that stack variables which require it.
         for (var_stack_offs, var_word_size, var_data_id) in init_mut_vars {
             // Load our initialiser from the data section.

diff --git a/sway-core/src/asm_generation/fuel/register_allocator.rs b/sway-core/src/asm_generation/fuel/register_allocator.rs
@@ -839,7 +839,8 @@ fn spill(ops: &[Op], spills: &FxHashSet<VirtualRegister>) -> Vec<Op> {
         .map(|(i, reg)| (reg.clone(), (i * 8) as u32 + locals_size_bytes))
         .collect();
 
-    let new_locals_byte_size = locals_size_bytes + (8 * spills.len()) as u32;
+    let spills_size = (8 * spills.len()) as u32;
+    let new_locals_byte_size = locals_size_bytes + spills_size;
     if new_locals_byte_size > compiler_constants::TWENTY_FOUR_BITS as u32 {
         panic!("Enormous stack usage for locals.");
     }
@@ -851,8 +852,7 @@ fn spill(ops: &[Op], spills: &FxHashSet<VirtualRegister>) -> Vec<Op> {
                 opcode: Either::Left(VirtualOp::CFEI(VirtualImmediate24 {
                     value: new_locals_byte_size,
                 })),
-                comment: op.comment.clone()
-                    + &format!(" and {new_locals_byte_size} bytes for spills"),
+                comment: op.comment.clone() + &format!(" and {spills_size} bytes for spills"),
                 owning_span: op.owning_span.clone(),
             });
         } else if matches!(cfs_idx_opt, Some(cfs_idx) if cfs_idx == op_idx) {
@@ -861,8 +861,7 @@ fn spill(ops: &[Op], spills: &FxHashSet<VirtualRegister>) -> Vec<Op> {
                 opcode: Either::Left(VirtualOp::CFSI(VirtualImmediate24 {
                     value: new_locals_byte_size,
                 })),
-                comment: op.comment.clone()
-                    + &format!(" and {new_locals_byte_size} bytes for spills"),
+                comment: op.comment.clone() + &format!(" and {spills_size} bytes for spills"),
                 owning_span: op.owning_span.clone(),
             });
         } else {

diff --git a/sway-ir/src/optimize/dce.rs b/sway-ir/src/optimize/dce.rs
@@ -206,6 +206,17 @@ pub fn dce(
     let mut num_symbol_uses: HashMap<Symbol, u32> = HashMap::new();
     let mut stores_of_sym: HashMap<Symbol, Vec<Value>> = HashMap::new();
 
+    // Every argument is assumed to be loaded from (from the caller),
+    // so stores to it shouldn't be deliminated.
+    for sym in function
+        .args_iter(context)
+        .flat_map(|arg| get_symbols(context, arg.1))
+    {
+        num_symbol_uses
+            .entry(sym)
+            .and_modify(|count| *count += 1)
+            .or_insert(1);
+    }
     // Go through each instruction and update use_count.
     for (_block, inst) in function.instruction_iter(context) {
         for sym in get_loaded_symbols(context, inst) {

diff --git a/sway-ir/src/optimize/inline.rs b/sway-ir/src/optimize/inline.rs
@@ -151,17 +151,6 @@ pub fn inline_in_module(
             return true;
         }
 
-        // As per https://github.com/FuelLabs/sway/issues/2819 we can hit problems if a function
-        // argument is used as a pointer (probably because it has a ref type) although it actually
-        // isn't one.  Ref type args which aren't pointers need to be inlined.
-        if func.args_iter(ctx).any(|(_name, arg_val)| {
-            arg_val.get_type(ctx).map_or(false, |ty| {
-                ty.is_ptr(ctx) || !(ty.is_unit(ctx) | ty.is_bool(ctx) | ty.is_uint(ctx))
-            })
-        }) {
-            return true;
-        }
-
         false
     };
 

diff --git a/sway-ir/tests/dce/dce3.ir b/sway-ir/tests/dce/dce3.ir
@@ -0,0 +1,39 @@
+script {
+    entry fn main() -> bool {
+        local mut u64 state
+
+        entry():
+        v0 = get_local ptr u64, state
+        v1 = const u64 0
+        store v1 to v0
+        v2 = get_local ptr u64, state
+        v3 = call function_0(v2)
+        v4 = get_local ptr u64, state
+        v5 = load v4
+        v6 = const u64 42
+        v7 = cmp eq v5 v6
+        v8 = const bool false
+        v9 = cmp eq v7 v8
+        cbr v9, assert_1_block0(), assert_1_block1()
+
+        assert_1_block0():
+        v10 = const u64 18446744073709486084
+        revert v10
+
+        assert_1_block1():
+        v11 = const bool true
+        ret bool v11
+    }
+
+    // check: function_0
+    fn function_0(state: ptr u64) -> () {
+        entry(state: ptr u64):
+        v0 = const u64 42
+        // check: store
+        store v0 to state
+        v1 = const unit ()
+        // not: add
+        v2 = add v0, v0
+        ret () v1
+    }
+}
diff --git a/.../e2e_vm_tests/test_programs/should_pass/language/configurable_consts/json_abi_oracle.json b/.../e2e_vm_tests/test_programs/should_pass/language/configurable_consts/json_abi_oracle.json
@@ -7,7 +7,7 @@
         "typeArguments": null
       },
       "name": "C0",
-      "offset": 4212
+      "offset": 4180
     },
     {
       "configurableType": {
@@ -16,7 +16,7 @@
         "typeArguments": null
       },
       "name": "C1",
-      "offset": 4220
+      "offset": 4188
     },
     {
       "configurableType": {
@@ -25,7 +25,7 @@
         "typeArguments": null
       },
       "name": "C2",
-      "offset": 4236
+      "offset": 4204
     },
     {
       "configurableType": {
@@ -34,7 +34,7 @@
         "typeArguments": []
       },
       "name": "C3",
-      "offset": 4268
+      "offset": 4236
     },
     {
       "configurableType": {
@@ -43,7 +43,7 @@
         "typeArguments": []
       },
       "name": "C4",
-      "offset": 4284
+      "offset": 4252
     },
     {
       "configurableType": {
@@ -52,7 +52,7 @@
         "typeArguments": []
       },
       "name": "C5",
-      "offset": 4300
+      "offset": 4268
     },
     {
       "configurableType": {
@@ -61,7 +61,7 @@
         "typeArguments": null
       },
       "name": "C6",
-      "offset": 4316
+      "offset": 4284
     },
     {
       "configurableType": {
@@ -70,7 +70,7 @@
         "typeArguments": null
       },
       "name": "C7",
-      "offset": 4332
+      "offset": 4300
     },
     {
       "configurableType": {
@@ -79,7 +79,7 @@
         "typeArguments": null
       },
       "name": "C9",
-      "offset": 4380
+      "offset": 4348
     }
   ],
   "functions": [

diff --git a/...test_programs/should_pass/require_contract_deployment/array_of_structs_caller/src/main.sw b/...test_programs/should_pass/require_contract_deployment/array_of_structs_caller/src/main.sw
@@ -4,7 +4,7 @@ use array_of_structs_abi::{Id, TestContract, Wrapper};
 use std::hash::sha256;
 
 fn main() -> u64 {
-    let addr = abi(TestContract, 0x03d3ef50c3cf3716962cd0a447c68c3f2c85b980425e4b313c275dd1da28de8b);
+    let addr = abi(TestContract, 0xb534f555ed15c5f99dc318f4260faeba317ecefa094aba85c5c5e7ad043480ad);
 
     let input = [Wrapper {
         id: Id {

diff --git a/...ts/test_programs/should_pass/require_contract_deployment/call_abi_with_tuples/src/main.sw b/...ts/test_programs/should_pass/require_contract_deployment/call_abi_with_tuples/src/main.sw
@@ -3,7 +3,7 @@ script;
 use abi_with_tuples::*;
 
 fn main() -> bool {
-    let the_abi = abi(MyContract, 0x6865d45808bc8127ecfaf6ad4376175cd9d9cdddcc10e9aa3a62228e690e540e);
+    let the_abi = abi(MyContract, 0x553558ff0c47820e5012953153ec30658bd2df11ef013139839551579dbff2f0);
 
     let param1 = (
         Person {

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
@@ -2,7 +2,7 @@ script;
 use basic_storage_abi::{BasicStorage, Quad};
 
 fn main() -> u64 {
-    let addr = abi(BasicStorage, 0xb3c7a231544c293a5a6bca7817591b567800242079df4b48657f9c8321fbc861);
+    let addr = abi(BasicStorage, 0xd32372eac35fc0d05d1cbefb6d5f6a51f14ea024e4cb452ed3faa99b4ddb165d);
     let key = 0x0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
     let value = 4242;
 

diff --git a/...grams/should_pass/require_contract_deployment/call_contract_with_type_aliases/src/main.sw b/...grams/should_pass/require_contract_deployment/call_contract_with_type_aliases/src/main.sw
@@ -3,7 +3,7 @@ script;
 use contract_with_type_aliases_abi::*;
 
 fn main() {
-    let caller = abi(MyContract, 0x1de6828181093516b4421511a953ae9603b31672345a88eda698c3ab0f021fd6);
+    let caller = abi(MyContract, 0x327fe8d236dd68c93d9e16e3a28fb3ab45404fa75b44e6ebf435e74984cd56fd);
 
     let x = AssetId::from(0x0101010101010101010101010101010101010101010101010101010101010101);
 

diff --git a/...test_programs/should_pass/require_contract_deployment/call_increment_contract/src/main.sw b/...test_programs/should_pass/require_contract_deployment/call_increment_contract/src/main.sw
@@ -3,7 +3,7 @@ script;
 use increment_abi::Incrementor;
 
 fn main() -> bool {
-    let the_abi = abi(Incrementor, 0xad32919ed5a3c4dd21a79d35f3615c4c36ca76ff10c8bf6415bcc916838e9f6e);
+    let the_abi = abi(Incrementor, 0x0f18501abf5e6bb3117f73578dacbde5a51b0ba75d32d402e2e955e0c06a8112);
     the_abi.increment(5);
     the_abi.increment(5);
     let result = the_abi.get();

diff --git a/...sts/test_programs/should_pass/require_contract_deployment/caller_context_test/src/main.sw b/...sts/test_programs/should_pass/require_contract_deployment/caller_context_test/src/main.sw
@@ -7,7 +7,7 @@ fn main() -> bool {
     let zero = b256::min();
     let gas: u64 = u64::max();
     let amount: u64 = 11;
-    let other_contract_id = ContractId::from(0x98f8c4077c4f23d4def9693f8464fc4bf83b2295735cf2cfd027d99b9d4bbdcc);
+    let other_contract_id = ContractId::from(0x8049358afbe3e0c919d595aab87074ad6e192ae6e13058fb0f15fc5ca09da66d);
     let base_asset_id = BASE_ASSET_ID;
 
     let test_contract = abi(ContextTesting, other_contract_id.into());

diff --git a/...st_programs/should_pass/require_contract_deployment/nested_struct_args_caller/src/main.sw b/...st_programs/should_pass/require_contract_deployment/nested_struct_args_caller/src/main.sw
@@ -3,7 +3,7 @@ script;
 use nested_struct_args_abi::*;
 
 fn main() -> bool {
-    let contract_id = 0xa16f173a7e49805781eeb4b797fad3debb42a443c4208555904a106b43a75368;
+    let contract_id = 0x33639d15e9187676324cf8a42f6e15349660dc04d4cd52a9919bb9b3f15f732e;
     let caller = abi(NestedStructArgs, contract_id);
 
     let param_one = StructOne {

diff --git a/...s/test_programs/should_pass/require_contract_deployment/storage_access_caller/src/main.sw b/...s/test_programs/should_pass/require_contract_deployment/storage_access_caller/src/main.sw
@@ -4,7 +4,7 @@ use storage_access_abi::*;
 use std::hash::sha256;
 
 fn main() -> bool {
-    let contract_id = 0x37cb5e67d5e1b8c1489ac20ce7dd2868fd2f49e4f3ec44224fa717ceeae357b2;
+    let contract_id = 0x1838d1e56392f46b1da5620c81cbb0d371520365dee7300b56112e74dd6fc24d;
     let caller = abi(StorageAccess, contract_id);
 
     // Test initializers

diff --git a/...vm_tests/test_programs/should_pass/require_contract_deployment/token_ops_test/src/main.sw b/...vm_tests/test_programs/should_pass/require_contract_deployment/token_ops_test/src/main.sw
@@ -14,7 +14,7 @@ fn main() -> bool {
     let default_gas = 1_000_000_000_000;
 
     // the deployed fuel_coin Contract_Id:
-    let fuelcoin_id = ContractId::from(0x27fbd9fe28325217617106cde4dbb5a4713a7e54ea9adc0ee111c0aea1c2b799);
+    let fuelcoin_id = ContractId::from(0x1965bb988d9f2d052ee947f5ec33a5cba8f140cee7cf1ef084215c12915709e2);
 
     // contract ID for sway/test/src/e2e_vm_tests/test_programs/should_pass/test_contracts/balance_test_contract/
     let balance_test_id = ContractId::from(0xa49cbeebc26a586897a895092ee2bc138c982ee31404e7bc9d2c1d9bbec6e036);

diff --git a/test/src/ir_generation/tests/fn_call.sw b/test/src/ir_generation/tests/fn_call.sw
@@ -48,11 +48,12 @@ fn main() -> u64 {
 //
 // Matching fn b() here, which has a local bool var, initialised to false/$zero:
 //
+// check: move $$$$locbase $$sp
+// check: cfei i8
+//
 // check: move $REG $$$$arg0
 // check: move $REG $$$$arg1
 //
-// check: move $$$$locbase $$sp
-// check: cfei i8
 // check: sw   $$$$locbase $$zero i0
 // ...
 // check: cfsi i8