Skip to content

Commit

Permalink
Fix generic function dep defect; rework function discovery approach. (a…
Browse files Browse the repository at this point in the history 
…nza-xyz#199)

We have previously discovered through experience that some of the model-provided
information we once depended on to discover all module functions, called functions,
and concrete instantiations are not always consistent or reliable.

For this reason, we now take a different approach and seed our discovery with just the
list of functions provided by `ModuleEnv::get_functions`. For any other called functions
(this module or foreign) and for any generic instantiations, we will expand the seed
frontier incrementally by gleaning the remaining information from a visitation of every
function call instruction (recursively) in every seed function.

This also fixes a dependence problem in the previous discovery approach where some
transitively expanded generics were never seen or declared.

No new tests were added as the previous generics work covers this. We now, however,
can eliminate the dependence defect work around that was originally in the moption01
test case. It now runs without the work around successfully.
  • Loading branch information
@nvjle @ksolana
nvjle authored and ksolana committed Jul 6, 2023
1 parent e2085de commit 54548d9
Show file tree
Hide file tree
Showing 6 changed files with 146 additions and 153 deletions.
202 changes: 102 additions & 100 deletions language/tools/move-mv-llvm-compiler/src/stackless/translate.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,122 +448,121 @@ impl<'mm, 'up> ModuleContext<'mm, 'up> {
/// Create LLVM function decls for all local functions and
/// all extern functions that might be called.
fn declare_functions(&mut self) {
use move_binary_format::access::ModuleAccess;
let mod_env = self.env.clone(); // fixme bad clone

// We have previously discovered through experience that some of the model-provided
// information we once depended on to discover all module functions, called functions,
// and concrete instantiations are not always consistent or reliable.
//
// For this reason, we now take a different approach and seed our discovery with just the
// list of functions provided by `ModuleEnv::get_functions`. For any other called functions
// (this module or foreign) and for any generic instantiations, we will expand the seed
// frontier incrementally by gleaning the remaining information from a visitation of every
// function call instruction (recursively) in every seed function.
//
// While this results in yet another linear walk over all the code, it seems to be the
// simplest way to work around the model inconsistencies.
for fn_env in mod_env.get_functions() {
self.declare_functions_walk(&mod_env, &fn_env, vec![]);
}
}

fn declare_functions_walk(
&mut self,
mod_env: &mm::ModuleEnv,
curr_fn_env: &mm::FunctionEnv,
curr_type_vec: Vec<mty::Type>,
) {
let g_env = &mod_env.env;

let mut foreign_fns = BTreeSet::new();
let mut fn_instantiations: BTreeMap<mm::QualifiedId<mm::FunId>, Vec<Vec<mty::Type>>> =
BTreeMap::new();
// Do not process a previously declared function/expansion.
let fn_name = if curr_fn_env.is_native() {
curr_fn_env.llvm_native_fn_symbol_name()
} else if curr_fn_env.get_type_parameter_count() == 0 {
curr_fn_env.llvm_symbol_name(&[])
} else {
curr_fn_env.llvm_symbol_name(&curr_type_vec)
};

// First collect any generic instantiations and map to qualified ids.
// Each generic function handle represents potentially multiple concrete functions.
// This map supplies the type parameter vector for each concrete instance.
let this_module_data = &g_env.module_data[mod_env.get_id().to_usize()];
let cm = &this_module_data.module;
for fi_view in cm.function_instantiations() {
let tyvec = mod_env.get_type_actuals(Some(fi_view.type_parameters));
let fn_env = mod_env.get_used_function(fi_view.handle);
fn_instantiations
.entry(fn_env.get_qualified_id())
.or_insert_with(Vec::new)
.push(tyvec);
if self.fn_decls.get(&fn_name).is_some() {
return;
}

for fn_env in mod_env.get_functions() {
let linkage = fn_env.llvm_linkage();

// For native functions and concrete Move functions, declare a single function.
//
// For a generic Move function, declare all concrete expansions. The generic function
// itself will not be emitted.
let fn_qid = fn_env.get_qualified_id();
if !fn_env.is_native() && fn_env.get_type_parameter_count() > 0 {
if !fn_instantiations.contains_key(&fn_qid) {
continue;
}
for fi in &fn_instantiations[&fn_qid] {
// Do not attempt to instantiate generics whose type parameters themselves
// are generic. Those cannot be expanded until a function containing them
// is instantiated, resolving the type parameters.
let inst_is_generic = fi.iter().any(|t| t.is_open());
if inst_is_generic {
continue;
}
self.declare_function(&fn_env, fi, linkage);
let fn_qiid = fn_qid.module_id.qualified_inst(fn_qid.id, fi.to_vec());
self.expanded_functions.push(fn_qiid);
}
} else {
self.declare_function(&fn_env, &[], linkage);
let fn_qiid = fn_qid.module_id.qualified_inst(fn_qid.id, vec![]);
if !fn_env.is_native() {
self.expanded_functions.push(fn_qiid);
}
}
let fn_data = StacklessBytecodeGenerator::new(curr_fn_env).generate_function();

for called_fn in fn_env.get_transitive_closure_of_called_functions() {
// Pull in not just the directly called functions, but the transitive closure
// of called functions. Note that all directly called functions that are not
// in our module are public by definition. But those can transitively invoke
// private functions outside of our module, so exclude those.
let is_foreign_mod = called_fn.module_id != mod_env.get_id();
if is_foreign_mod && g_env.get_function(called_fn).is_exposed() {
foreign_fns.insert(called_fn);
}
}
// If the current function is either a native function or a concrete Move function,
// we have all the information needed to declare a corresponding single function.
//
// If the current function is a generic Move function, we will defer declaring its
// concrete expansions until a call path leading to a particular call site is visited.
// At that point, the type parameters are either resolved or the function is not used
// in the module. The generic function itself will not be emitted.
let curr_fn_qid = curr_fn_env.get_qualified_id();
if curr_fn_env.is_native() {
// Declare the native and return early--- there is no function body to visit.
self.declare_native_function(curr_fn_env, &fn_data, curr_fn_env.llvm_linkage());
return;
} else if curr_fn_env.get_type_parameter_count() == 0 {
let curr_fn_qiid = curr_fn_qid.module_id.qualified_inst(curr_fn_qid.id, vec![]);
self.declare_move_function(curr_fn_env, &[], &fn_data, curr_fn_env.llvm_linkage());
if curr_fn_qid.module_id != mod_env.get_id() {
// True foreign functions are only declared in our module, don't process further.
return;
}
self.expanded_functions.push(curr_fn_qiid);
} else {
// Determine whether any of the type parameters for this generic function are still
// unresolved. If so, then function is not a concrete instance and we defer it until
// a call path containing it is expanded.
assert!(curr_fn_env.get_type_parameter_count() > 0);
let inst_is_generic = curr_type_vec.iter().any(|t| t.is_open());
if curr_type_vec.is_empty() || inst_is_generic {
return;
}

// Note that we may be declaring a foreign function here. But since it is being
// expanded into our current module, its linkage is effectively private.
let curr_fn_qiid = curr_fn_qid
.module_id
.qualified_inst(curr_fn_qid.id, curr_type_vec.clone());
self.declare_move_function(
curr_fn_env,
&curr_type_vec,
&fn_data,
llvm::LLVMLinkage::LLVMPrivateLinkage,
);
self.expanded_functions.push(curr_fn_qiid);
}

for fn_qid in foreign_fns {
let called_fn_env = g_env.get_function(fn_qid);
if !called_fn_env.is_native() && called_fn_env.get_type_parameter_count() > 0 {
if !fn_instantiations.contains_key(&fn_qid) {
continue;
}
for fi in &fn_instantiations[&fn_qid] {
// Do not attempt to instantiate generics whose type parameters themselves
// are generic. Those cannot be expanded until a function containing them
// is instantiated, resolving the type parameters.
let inst_is_generic = fi.iter().any(|t| t.is_open());
if inst_is_generic {
continue;
}
self.declare_function(
&called_fn_env,
fi,
llvm::LLVMLinkage::LLVMPrivateLinkage,
);
let fn_qiid = fn_qid.module_id.qualified_inst(fn_qid.id, fi.to_vec());
assert!(called_fn_env.get_qualified_id() == fn_qid);
self.expanded_functions.push(fn_qiid);
}
} else {
self.declare_function(&called_fn_env, &[], called_fn_env.llvm_linkage());
// Visit every call site in the current function, instantiate their type parameters,
// and then recursively grow the frontier.
for instr in &fn_data.code {
if let sbc::Bytecode::Call(
_,
_,
sbc::Operation::Function(mod_id, fun_id, types),
_,
None,
) = instr
{
// Instantiate any type parameters at the current call site with the
// enclosing type parameter scope `curr_type_vec`.
let types = mty::Type::instantiate_vec(types.to_vec(), &curr_type_vec);

// Recursively discover/declare more functions on this call path.
let called_fn_env = g_env.get_function((*mod_id).qualified(*fun_id));
self.declare_functions_walk(mod_env, &called_fn_env, types);
}
}
}

fn declare_function(
&mut self,
fn_env: &mm::FunctionEnv,
tyvec: &[mty::Type],
linkage: llvm::LLVMLinkage,
) {
if fn_env.is_native() {
self.declare_native_function(fn_env, linkage)
} else {
self.declare_move_function(fn_env, tyvec, linkage)
}
}

fn declare_move_function(
&mut self,
fn_env: &mm::FunctionEnv,
tyvec: &[mty::Type],
fn_data: &FunctionData,
linkage: llvm::LLVMLinkage,
) {
let fn_data = StacklessBytecodeGenerator::new(fn_env).generate_function();

let ll_sym_name = fn_env.llvm_symbol_name(tyvec);
let ll_fn = {
let ll_fnty = {
Expand Down Expand Up @@ -610,11 +609,14 @@ impl<'mm, 'up> ModuleContext<'mm, 'up> {
/// At some point we might want to factor out the platform-specific ABI
/// decisions, but for now there are only a few ABI concerns, and we may
/// never support another platform for which the ABI is different.
fn declare_native_function(&mut self, fn_env: &mm::FunctionEnv, linkage: llvm::LLVMLinkage) {
fn declare_native_function(
&mut self,
fn_env: &mm::FunctionEnv,
fn_data: &FunctionData,
linkage: llvm::LLVMLinkage,
) {
assert!(fn_env.is_native());

let fn_data = StacklessBytecodeGenerator::new(fn_env).generate_function();

let ll_native_sym_name = fn_env.llvm_native_fn_symbol_name();
let ll_fn = {
let ll_fnty = {
Expand Down
24 changes: 12 additions & 12 deletions ...s/move-mv-llvm-compiler/tests/move-ir-tests/generic-func01-build/modules/0_M2.expected.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,32 +18,32 @@ entry:
ret %struct.M2__Coin_M2__Bitcoin_ %retval1
}

define %struct.M2__Coin_M2__Sol_ @M2__mint_concrete(i64 %0) {
define private %struct.M2__Coin_M2__Bitcoin_ @M2__mint_generic_M2__Bitcoin(i64 %0) {
entry:
%local_0 = alloca i64, align 8
%local_1__value = alloca i64, align 8
%local_2 = alloca %struct.M2__Coin_M2__Sol_, align 8
%local_2 = alloca %struct.M2__Coin_M2__Bitcoin_, align 8
store i64 %0, ptr %local_0, align 4
%load_store_tmp = load i64, ptr %local_0, align 4
store i64 %load_store_tmp, ptr %local_1__value, align 4
%fv.0 = load i64, ptr %local_1__value, align 4
%insert_0 = insertvalue %struct.M2__Coin_M2__Sol_ undef, i64 %fv.0, 0
store %struct.M2__Coin_M2__Sol_ %insert_0, ptr %local_2, align 4
%retval = load %struct.M2__Coin_M2__Sol_, ptr %local_2, align 4
ret %struct.M2__Coin_M2__Sol_ %retval
%insert_0 = insertvalue %struct.M2__Coin_M2__Bitcoin_ undef, i64 %fv.0, 0
store %struct.M2__Coin_M2__Bitcoin_ %insert_0, ptr %local_2, align 4
%retval = load %struct.M2__Coin_M2__Bitcoin_, ptr %local_2, align 4
ret %struct.M2__Coin_M2__Bitcoin_ %retval
}

define %struct.M2__Coin_M2__Bitcoin_ @M2__mint_generic_M2__Bitcoin(i64 %0) {
define %struct.M2__Coin_M2__Sol_ @M2__mint_concrete(i64 %0) {
entry:
%local_0 = alloca i64, align 8
%local_1__value = alloca i64, align 8
%local_2 = alloca %struct.M2__Coin_M2__Bitcoin_, align 8
%local_2 = alloca %struct.M2__Coin_M2__Sol_, align 8
store i64 %0, ptr %local_0, align 4
%load_store_tmp = load i64, ptr %local_0, align 4
store i64 %load_store_tmp, ptr %local_1__value, align 4
%fv.0 = load i64, ptr %local_1__value, align 4
%insert_0 = insertvalue %struct.M2__Coin_M2__Bitcoin_ undef, i64 %fv.0, 0
store %struct.M2__Coin_M2__Bitcoin_ %insert_0, ptr %local_2, align 4
%retval = load %struct.M2__Coin_M2__Bitcoin_, ptr %local_2, align 4
ret %struct.M2__Coin_M2__Bitcoin_ %retval
%insert_0 = insertvalue %struct.M2__Coin_M2__Sol_ undef, i64 %fv.0, 0
store %struct.M2__Coin_M2__Sol_ %insert_0, ptr %local_2, align 4
%retval = load %struct.M2__Coin_M2__Sol_, ptr %local_2, align 4
ret %struct.M2__Coin_M2__Sol_ %retval
}
56 changes: 28 additions & 28 deletions .../move-mv-llvm-compiler/tests/move-ir-tests/generic-func02-build/modules/1_M11.expected.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,19 @@ entry:
ret i64 %retval1
}

define private i64 @Coins__get_value_generic_M11__USDC(%struct.Coins__Coin_M11__USDC_ %0) {
entry:
%local_0 = alloca %struct.Coins__Coin_M11__USDC_, align 8
%local_1 = alloca %struct.Coins__Coin_M11__USDC_, align 8
%local_2__value = alloca i64, align 8
store %struct.Coins__Coin_M11__USDC_ %0, ptr %local_0, align 4
%srcval = load %struct.Coins__Coin_M11__USDC_, ptr %local_0, align 4
%ext_0 = extractvalue %struct.Coins__Coin_M11__USDC_ %srcval, 0
store i64 %ext_0, ptr %local_2__value, align 4
%retval = load i64, ptr %local_2__value, align 4
ret i64 %retval
}

define private { %struct.Coins__Coin_M11__USDC_, %struct.Coins__Coin_M11__Eth_ } @M11__mint_2coins_usdc_and_eth(i64 %0, i64 %1) {
entry:
%local_0 = alloca i64, align 8
Expand Down Expand Up @@ -47,34 +60,6 @@ entry:
ret { %struct.Coins__Coin_M11__USDC_, %struct.Coins__Coin_M11__Eth_ } %insert_1
}

define private %struct.Coins__Coin_M11__USDC_ @M11__mint_usdc(i64 %0) {
entry:
%local_0 = alloca i64, align 8
%local_1 = alloca i64, align 8
%local_2 = alloca %struct.Coins__Coin_M11__USDC_, align 8
store i64 %0, ptr %local_0, align 4
%load_store_tmp = load i64, ptr %local_0, align 4
store i64 %load_store_tmp, ptr %local_1, align 4
%call_arg_0 = load i64, ptr %local_1, align 4
%retval = call %struct.Coins__Coin_M11__USDC_ @Coins__mint_generic_M11__USDC(i64 %call_arg_0)
store %struct.Coins__Coin_M11__USDC_ %retval, ptr %local_2, align 4
%retval1 = load %struct.Coins__Coin_M11__USDC_, ptr %local_2, align 4
ret %struct.Coins__Coin_M11__USDC_ %retval1
}

define private i64 @Coins__get_value_generic_M11__USDC(%struct.Coins__Coin_M11__USDC_ %0) {
entry:
%local_0 = alloca %struct.Coins__Coin_M11__USDC_, align 8
%local_1 = alloca %struct.Coins__Coin_M11__USDC_, align 8
%local_2__value = alloca i64, align 8
store %struct.Coins__Coin_M11__USDC_ %0, ptr %local_0, align 4
%srcval = load %struct.Coins__Coin_M11__USDC_, ptr %local_0, align 4
%ext_0 = extractvalue %struct.Coins__Coin_M11__USDC_ %srcval, 0
store i64 %ext_0, ptr %local_2__value, align 4
%retval = load i64, ptr %local_2__value, align 4
ret i64 %retval
}

define private { %struct.Coins__Coin_M11__USDC_, %struct.Coins__Coin_M11__Eth_ } @Coins__mint_2coins_generic_M11__USDC_M11__Eth(i64 %0, i64 %1) {
entry:
%local_0 = alloca i64, align 8
Expand Down Expand Up @@ -102,6 +87,21 @@ entry:
ret { %struct.Coins__Coin_M11__USDC_, %struct.Coins__Coin_M11__Eth_ } %insert_1
}

define private %struct.Coins__Coin_M11__USDC_ @M11__mint_usdc(i64 %0) {
entry:
%local_0 = alloca i64, align 8
%local_1 = alloca i64, align 8
%local_2 = alloca %struct.Coins__Coin_M11__USDC_, align 8
store i64 %0, ptr %local_0, align 4
%load_store_tmp = load i64, ptr %local_0, align 4
store i64 %load_store_tmp, ptr %local_1, align 4
%call_arg_0 = load i64, ptr %local_1, align 4
%retval = call %struct.Coins__Coin_M11__USDC_ @Coins__mint_generic_M11__USDC(i64 %call_arg_0)
store %struct.Coins__Coin_M11__USDC_ %retval, ptr %local_2, align 4
%retval1 = load %struct.Coins__Coin_M11__USDC_, ptr %local_2, align 4
ret %struct.Coins__Coin_M11__USDC_ %retval1
}

define private %struct.Coins__Coin_M11__USDC_ @Coins__mint_generic_M11__USDC(i64 %0) {
entry:
%local_0 = alloca i64, align 8
Expand Down
2 changes: 0 additions & 2 deletions ...ols/move-mv-llvm-compiler/tests/move-ir-tests/multi-module-build/scripts/main.expected.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,4 @@ entry:
ret void
}

declare i8 @Test2__test2(i8, i8)

declare i8 @Test1__test1(i8, i8)
8 changes: 4 additions & 4 deletions ...ools/move-mv-llvm-compiler/tests/move-ir-tests/struct02-build/modules/1_UseIt.expected.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,12 @@ entry:
ret void
}

declare i8 @Country__dropit(%struct.Country__Country)

declare i8 @Country__get_id(ptr)
declare %struct.Country__Country @Country__new_country(i8, i64)

declare i64 @Country__get_pop(%struct.Country__Country)

declare %struct.Country__Country @Country__new_country(i8, i64)
declare i8 @Country__get_id(ptr)

declare void @Country__set_id(ptr, i8)

declare i8 @Country__dropit(%struct.Country__Country)
7 changes: 0 additions & 7 deletions language/tools/move-mv-llvm-compiler/tests/rbpf-tests/moption01.move
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,13 +301,6 @@ module 0x300::option_tests {
use 0x10::option;
use 0x10::vector;

fun phony() {
// Work around dependency problem. Remove when that is gone.
let v = vector::singleton(31);
assert!(vector::contains(&v, &31), 0);
assert!(vector::is_empty(&vector::empty<u64>()), 0);
}

public fun option_none_is_none() {
let none = option::none<u64>();
assert!(option::is_none(&none), 0);
Expand Down

0 comments on commit 54548d9

Please sign in to comment.