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

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

Merged
merged 12 commits into from
Jul 21, 2023
30 changes: 26 additions & 4 deletions sway-core/src/asm_generation/fuel/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*,
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
9 changes: 4 additions & 5 deletions sway-core/src/asm_generation/fuel/register_allocator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
}
Expand All @@ -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) {
Expand All @@ -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 {
Expand Down
11 changes: 11 additions & 0 deletions sway-ir/src/optimize/dce.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
11 changes: 0 additions & 11 deletions sway-ir/src/optimize/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
};

Expand Down
39 changes: 39 additions & 0 deletions sway-ir/tests/dce/dce3.ir
Original file line number Diff line number Diff line change
@@ -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
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"typeArguments": null
},
"name": "C0",
"offset": 4212
"offset": 4180
},
{
"configurableType": {
Expand All @@ -16,7 +16,7 @@
"typeArguments": null
},
"name": "C1",
"offset": 4220
"offset": 4188
},
{
"configurableType": {
Expand All @@ -25,7 +25,7 @@
"typeArguments": null
},
"name": "C2",
"offset": 4236
"offset": 4204
},
{
"configurableType": {
Expand All @@ -34,7 +34,7 @@
"typeArguments": []
},
"name": "C3",
"offset": 4268
"offset": 4236
},
{
"configurableType": {
Expand All @@ -43,7 +43,7 @@
"typeArguments": []
},
"name": "C4",
"offset": 4284
"offset": 4252
},
{
"configurableType": {
Expand All @@ -52,7 +52,7 @@
"typeArguments": []
},
"name": "C5",
"offset": 4300
"offset": 4268
},
{
"configurableType": {
Expand All @@ -61,7 +61,7 @@
"typeArguments": null
},
"name": "C6",
"offset": 4316
"offset": 4284
},
{
"configurableType": {
Expand All @@ -70,7 +70,7 @@
"typeArguments": null
},
"name": "C7",
"offset": 4332
"offset": 4300
},
{
"configurableType": {
Expand All @@ -79,7 +79,7 @@
"typeArguments": null
},
"name": "C9",
"offset": 4380
"offset": 4348
}
],
"functions": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
5 changes: 3 additions & 2 deletions test/src/ir_generation/tests/fn_call.sw
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading