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

RFC: Fix dyncalls api #13289

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion emcc.py
Original file line number Diff line number Diff line change
Expand Up @@ -1282,6 +1282,10 @@ def default_setting(name, new_default):
if shared.Settings.CLOSURE_WARNINGS not in ['quiet', 'warn', 'error']:
exit_with_error('Invalid option -s CLOSURE_WARNINGS=%s specified! Allowed values are "quiet", "warn" or "error".' % shared.Settings.CLOSURE_WARNINGS)

# Calling function pointers from JS libraries is default runtime functionality, so always include the functionality. (to be DCEd if not used)
if shared.Settings.WASM_DYNCALLS:
shared.Settings.DEFAULT_LIBRARY_FUNCS_TO_INCLUDE += ['$dynCall']

if shared.Settings.MAIN_MODULE:
assert not shared.Settings.SIDE_MODULE
if shared.Settings.MAIN_MODULE == 1:
Expand Down Expand Up @@ -1707,7 +1711,7 @@ def check_memory_setting(setting):
if shared.Settings.USE_PTHREADS or shared.Settings.RELOCATABLE or shared.Settings.ASYNCIFY_LAZY_LOAD_CODE or shared.Settings.WASM2JS:
shared.Settings.IMPORTED_MEMORY = 1

if shared.Settings.WASM_BIGINT:
if shared.Settings.WASM_BIGINT and not shared.Settings.WASM_DYNCALLS:
shared.Settings.LEGALIZE_JS_FFI = 0

shared.Settings.GENERATE_SOURCE_MAP = shared.Settings.DEBUG_LEVEL >= 4 and not shared.Settings.SINGLE_FILE
Expand Down Expand Up @@ -2420,11 +2424,13 @@ def consume_arg_file():
options.llvm_opts = ['-Os']
options.requested_level = 2
shared.Settings.SHRINK_LEVEL = 1
shared.Settings.USE_LEGACY_DYNCALLS = 0 # In -Os builds, use a more size compact, but slower 'wasmTable.get()' method of accessing function pointers
settings_changes.append('INLINING_LIMIT=50')
elif options.requested_level == 'z':
options.llvm_opts = ['-Oz']
options.requested_level = 2
shared.Settings.SHRINK_LEVEL = 2
shared.Settings.USE_LEGACY_DYNCALLS = 0 # In -Oz builds, use a more size compact, but slower 'wasmTable.get()' method of accessing function pointers
settings_changes.append('INLINING_LIMIT=25')
shared.Settings.OPT_LEVEL = validate_arg_level(options.requested_level, 3, 'Invalid optimization level: ' + arg, clamp=True)
elif check_arg('--js-opts'):
Expand Down
45 changes: 42 additions & 3 deletions emscripten.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,46 @@
WASM_INIT_FUNC = '__wasm_call_ctors'


def make_wasm_table_static_dyncaller(func):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is only used in minimal runtime? Don't we want to always generate these helpers on the JS side (when the user asks for them or needs them)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the intent is not to be minimal runtime only, but I just started looking at this in minimal runtime first. This may probably not be needed after all, I'll delete it soon if not.

sig = func.replace('dynCall_', '')
ret = '' if sig[0] == 'v' else 'return '
args = '' # 'a0, a1, a2, ..., aN'
ptr_args = '' # 'ptr, a0, a1, a2, ..., aN'
i = 0
while i < len(sig)-1:
if i > 0: args += ', '
args += 'a' + str(i)
i += 1
ptr_args = ('ptr, ' + args) if len(args) > 0 else 'ptr'

dyncall = ('dyncalls["' + sig + '"]') if shared.Settings.MINIMAL_RUNTIME else ('Module["' + func + '"]')
wasmTableGet = 'wasmTableGet' if shared.Settings.SHRINK_LEVEL == 0 else 'wasmTable.get'
return 'function ' + func + '(' + ptr_args + ') { ' + ret + dyncall + '(' + ptr_args + '); }\n'


def compute_minimal_runtime_initializer_and_exports(post, exports, receiving):
# Declare all exports out to global JS scope so that JS library functions can access them in a
# way that minifies well with Closure
# e.g. var a,b,c,d,e,f;
exports_that_are_not_initializers = [x for x in exports if x not in WASM_INIT_FUNC]
# In Wasm backend the exports are still unmangled at this point, so mangle the names here
exports_that_are_not_initializers = [asmjs_mangle(x) for x in exports_that_are_not_initializers]

static_dyncall_sig_functions = ''

if shared.Settings.WASM_DYNCALLS:
if len([x for x in exports_that_are_not_initializers if x.startswith('dynCall_')]) > 0:
exports_that_are_not_initializers += ['dynCalls = {}']
else:
for x in exports_that_are_not_initializers:
if x.startswith('dynCall_'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this assumes that dynCall_ functions are exported from the wasm? That isn't the case for WASM_BIGINT or otherwise when we do not legalize, and is definitely not the case when wasm-emscripten-finalize does not write out any changes at all to the wasm (then the wasm is identical to what LLVM emitted).

I think we need to make wasm-emscripten-finalize print out the list of function pointer types in the metadata.

static_dyncall_sig_functions += make_wasm_table_static_dyncaller(x) + '\n'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we are generating these dynCall_ function on demand (based on them being added explicitly to the export list by the user). If so that why would we ever want to add an option to disable them? Isn't there already no cost assuming you don't export any of them?

Can we land this part of the change now or get support for JS-side dynCall_xx functions? Hopefully this can be independent of any new APIs or options?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be ok with always generating them all, as long as optimizer DCEs the ones that are not invoked. In Unity we probably still want to export dynCall to Module, but the other dynCall_sig items would not be exported.


exports_that_are_not_initializers = [x for x in exports_that_are_not_initializers if not x.startswith('dynCall_')]

post = post.replace('/*** ASM_MODULE_EXPORTS_DECLARES ***/', 'var ' + ',\n '.join(exports_that_are_not_initializers) + ';')
# TODO: Check codegen again
# post = post.replace('/*** STATIC_DYNCALL_SIG_FUNCTIONS ***/', static_dyncall_sig_functions)

# Generate assignments from all asm.js/wasm exports out to the JS variables above: e.g. a = asm['a']; b = asm['b'];
post = post.replace('/*** ASM_MODULE_EXPORTS ***/', receiving)
Expand Down Expand Up @@ -399,9 +431,9 @@ def finalize_wasm(infile, outfile, memfile, DEBUG):
# (which matches what llvm+lld has given us)
if shared.Settings.DEBUG_LEVEL >= 2 or shared.Settings.ASYNCIFY_ADD or shared.Settings.ASYNCIFY_ADVISE or shared.Settings.ASYNCIFY_ONLY or shared.Settings.ASYNCIFY_REMOVE or shared.Settings.EMIT_SYMBOL_MAP or shared.Settings.PROFILING_FUNCS:
args.append('-g')
if shared.Settings.WASM_BIGINT:
if shared.Settings.WASM_BIGINT and not shared.Settings.WASM_DYNCALLS: # TODO: This may be troublematic?
args.append('--bigint')
if shared.Settings.USE_LEGACY_DYNCALLS:
if True:##shared.Settings.USE_LEGACY_DYNCALLS:
# we need to add all dyncalls to the wasm
modify_wasm = True
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you want to remove this entire if/else, and we can also remove the all the dynCall generation from binaryen? Or at least that would be really cool if we could.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have a user-facing option WASM_DYNCALLS, then this might be needed when that is enabled.

else:
Expand Down Expand Up @@ -698,6 +730,10 @@ def create_receiving(exports):

exports_that_are_not_initializers = [x for x in exports if x != WASM_INIT_FUNC]

# If we are not building with dynCall() support, filter out all the dynCall_ exports.
if not shared.Settings.WASM_DYNCALLS:
exports_that_are_not_initializers = [x for x in exports_that_are_not_initializers if not x.startswith('dynCall_')]

receiving = []

# with WASM_ASYNC_COMPILATION that asm object may not exist at this point in time
Expand All @@ -710,7 +746,10 @@ def create_receiving(exports):
# WebAssembly.instantiate(Module["wasm"], imports).then((function(output) {
# var asm = output.instance.exports;
# _main = asm["_main"];
receiving += [asmjs_mangle(s) + ' = asm["' + s + '"];' for s in exports_that_are_not_initializers]
for s in exports_that_are_not_initializers:
mangled = asmjs_mangle(s)
dynCallAssignment = ('dynCalls["' + s.replace('dynCall_', '') + '"] = ') if shared.Settings.WASM_DYNCALLS and mangled.startswith('dynCall_') else ''
receiving += [dynCallAssignment + mangled + ' = asm["' + s + '"];']
else:
if shared.Settings.MINIMAL_RUNTIME:
# In wasm2js exports can be directly processed at top level, i.e.
Expand Down
56 changes: 0 additions & 56 deletions src/library.js
Original file line number Diff line number Diff line change
Expand Up @@ -3696,62 +3696,6 @@ LibraryManager.library = {
});
},

#if USE_LEGACY_DYNCALLS || !WASM_BIGINT
$dynCallLegacy: function(sig, ptr, args) {
#if ASSERTIONS
assert(('dynCall_' + sig) in Module, 'bad function pointer type - no table for sig \'' + sig + '\'');
if (args && args.length) {
// j (64-bit integer) must be passed in as two numbers [low 32, high 32].
assert(args.length === sig.substring(1).replace(/j/g, '--').length);
} else {
assert(sig.length == 1);
}
#endif
if (args && args.length) {
return Module['dynCall_' + sig].apply(null, [ptr].concat(args));
}
return Module['dynCall_' + sig].call(null, ptr);
},
$dynCall__deps: ['$dynCallLegacy'],

// Used in library code to get JS function from wasm function pointer.
// All callers should use direct table access where possible and only fall
// back to this function if needed.
$getDynCaller__deps: ['$dynCall'],
$getDynCaller: function(sig, ptr) {
#if !USE_LEGACY_DYNCALLS
assert(sig.indexOf('j') >= 0, 'getDynCaller should only be called with i64 sigs')
#endif
var argCache = [];
return function() {
argCache.length = arguments.length;
for (var i = 0; i < arguments.length; i++) {
argCache[i] = arguments[i];
}
return dynCall(sig, ptr, argCache);
};
},
#endif

$dynCall: function(sig, ptr, args) {
#if USE_LEGACY_DYNCALLS
return dynCallLegacy(sig, ptr, args);
#else
#if !WASM_BIGINT
// Without WASM_BIGINT support we cannot directly call function with i64 as
// part of thier signature, so we rely the dynCall functions generated by
// wasm-emscripten-finalize
if (sig.indexOf('j') != -1) {
return dynCallLegacy(sig, ptr, args);
}
#endif
#if ASSERTIONS
assert(wasmTable.get(ptr), 'missing table entry in dynCall: ' + ptr);
#endif
return wasmTable.get(ptr).apply(null, args)
#endif
},

$callRuntimeCallbacks: function(callbacks) {
while(callbacks.length > 0) {
var callback = callbacks.shift();
Expand Down
78 changes: 78 additions & 0 deletions src/library_dyncall.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
mergeInto(LibraryManager.library, {
{{{ (function() { global.wbind = function() { return SHRINK_LEVEL == 0 ? 'wbind' : 'wasmTable.get'; }; return null; })(); }}}
{{{ (function() { global.getDynCaller = function(sig) { return MINIMAL_RUNTIME ? `dynCalls[${sig}]` : `Module["dynCall_"+${sig}]`; }; return null; })(); }}}

#if SHRINK_LEVEL == 0
// A mirror copy of contents of wasmTable in JS side, to avoid relatively
// slow wasmTable.get() call. Only used when not compiling with -Os or -Oz.
_wasmTableMirror: [],

$wbind__deps: ['_wasmTableMirror'],
$wbind: function(funcPtr) {
var func = __wasmTableMirror[funcPtr];
if (!func) {
if (funcPtr >= __wasmTableMirror.length) __wasmTableMirror.length = funcPtr + 1;
__wasmTableMirror[funcPtr] = func = wasmTable.get(funcPtr);
}
return func;
},

#if WASM_DYNCALLS
$dynCall__deps: ['$wbind'],
$bindDynCall__deps: ['$wbind'],
#endif
$wbindArray__deps: ['$wbind'],
#else
$wbind: function(funcPtr) {
// In -Os and -Oz builds, do not implement a JS side wasm table mirror for small
// code size, but directly access wasmTable, which is a bit slower.
return wasmTable.get(funcPtr);
},
#endif

// A helper that binds a wasm function into a form that can be called by passing all
// the parameters in an array, e.g. wbindArray(func)([param1, param2, ..., paramN]).
$wbindArray: function(funcPtr) {
var func = {{{wbind()}}}(funcPtr);
return func.length
? function(args) { return func.apply(null, args); }
: function() { return func(); }
},

#if WASM_DYNCALLS
// A helper that returns a function that can be used to invoke function pointers, i.e.
// getDynCaller('vi')(funcPtr, myInt);
$getDynCaller: function(sig, funcPtr) {
return {{{getDynCaller('sig')}}};
},

$bindDynCall: function(sig, funcPtr) {
// For int64 signatures, use the dynCall_sig dispatch mechanism.
if (sig.includes('j')) return function(args) {
return {{{getDynCaller('sig')}}}.apply(null, [funcPtr].concat(args));
}
// For non-int64 signatures, invoke via the wasm table.
var func = {{{wbind()}}}(funcPtr);
return func.length
? function(args) { return func.apply(null, args); }
: function() { return func(); }
},

#if SHRINK_LEVEL
$dynCall__deps: ['$bindDynCall'],
#endif
$dynCall: function(sig, funcPtr, args) {
#if SHRINK_LEVEL
return bindDynCall(sig, funcPtr)(args);
#else
// For int64 signatures, use the dynCall_sig dispatch mechanism.
if (sig.includes('j')) {
return {{{getDynCaller('sig')}}}.apply(null, [funcPtr].concat(args));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention in other library files seem to be to have white space inside the {{{ ... }}}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use just implement this in terms if bindDynCall above?

$dynCall: function(sig, funcPtr, args) { return bindDynCall(sig, funcPtr).apply(null, args); }

I imagine it would be a slight perf loss but a code size win, and I don't think the generic runtime dynCall like this should be on anyones critical path, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is reason to make this code suboptimal, it is just a few lines. But added a variant in -Os/-Oz to reuse the code to make it smaller.

}

// For non-int64 signatures, invoke via the wasm table.
return {{{wbind()}}}(funcPtr).apply(null, args);
#endif
},
#endif
});
1 change: 1 addition & 0 deletions src/modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ var LibraryManager = {
// Core system libraries (always linked against)
var libraries = [
'library.js',
'library_dyncall.js',
'library_stack.js',
'library_formatString.js',
'library_math.js',
Expand Down
5 changes: 4 additions & 1 deletion src/postamble_minimal.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ function loadWasmModuleToWorkers() {
/*** ASM_MODULE_EXPORTS_DECLARES ***/
#endif

/*** STATIC_DYNCALL_SIG_FUNCTIONS ***/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR but other places where do late substitution I've been using <<< ... >>> to avoid confusion with jsifier's {{{ ... }}}. Should we switch these to use the same pattern?

Seems like it would be good to distinguish comments from substitution patterns, and also will correctly error out of we forget/fail to make the substitution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah maybe <<< >>> is good, I'll have that in a followup. Although now I wonder if I need this at all. I'll see after I get through the test suite.



#if MINIMAL_RUNTIME_STREAMING_WASM_INSTANTIATION
// https://caniuse.com/#feat=wasm and https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/instantiateStreaming
// Firefox 52 added Wasm support, but only Firefox 58 added instantiateStreaming.
Expand Down Expand Up @@ -222,7 +225,7 @@ WebAssembly.instantiate(Module['wasm'], imports).then(function(output) {
initRuntime(asm);
#if USE_PTHREADS && PTHREAD_POOL_SIZE
if (!ENVIRONMENT_IS_PTHREAD) loadWasmModuleToWorkers();
#if !PTHREAD_POOL_DELAY_LOAD
#if !PTHREAD_POOL_DELAY_LOAD
else
#endif
ready();
Expand Down
4 changes: 4 additions & 0 deletions src/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -1145,6 +1145,10 @@ var BINARYEN_EXTRA_PASSES = "";
// (This option was formerly called BINARYEN_ASYNC_COMPILATION)
var WASM_ASYNC_COMPILATION = 1;

// If true, enables emitting dynCall() and dynCall_sig() based function pointer
// invokers to call function pointers from JS to Wasm.
var WASM_DYNCALLS = 1;

// WebAssembly integration with JavaScript BigInt. When enabled we don't need
// to legalize i64s into pairs of i32s, as the wasm VM will use a BigInt where
// an i64 is used.
Expand Down
2 changes: 1 addition & 1 deletion src/settings_internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ var EXPECT_MAIN = 1;
// MODULARIZE, and returned from the factory function.
var EXPORT_READY_PROMISE = 1;

var USE_LEGACY_DYNCALLS = 0;
var USE_LEGACY_DYNCALLS = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intended this setting to mean "use the dyncalls that are generated by binaryen". Perhaps we don't need those add anymore after thise change? In nay case can you add a comment here so its clear what the new meaning is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that may be needed in some form or the other. Let me see what form the code takes after test suite passes.


// struct_info that is either generated or cached
var STRUCT_INFO = '';
Expand Down
Loading