Skip to content

Remove table64 lowering #22452

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 4, 2024
Merged

Remove table64 lowering #22452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 31 additions & 3 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ commands:
description: "install canary version of node"
steps:
- install-node-version:
node_version: "22.0.0-v8-canary20240423a2eb2005bd"
node_version: "23.0.0-v8-canary20240819f52f1c2f1c"
canary: true
install-v8:
description: "install v8 using jsvu"
Expand Down Expand Up @@ -625,6 +625,12 @@ jobs:
- run: rm -rf $HOME/.jsvu
- install-v8
- install-node-canary
# When running wasm64 tests we need to make sure we use the testing
# version of node (node canary) when running the compiler output (e.g.
# in configure tests.
- run:
name: configure node canary
command: echo "NODE_JS = NODE_JS_TEST" >> ~/emsdk/.emscripten
- run-tests:
title: "wasm64"
test_targets: "
Expand All @@ -651,6 +657,12 @@ jobs:
- run: rm -rf $HOME/.jsvu
- install-v8
- install-node-canary
# When running wasm64 tests we need to make sure we use the testing
# version of node (node canary) when running the compiler output (e.g.
# in configure tests.
- run:
name: configure node canary
command: echo "NODE_JS = NODE_JS_TEST" >> ~/emsdk/.emscripten
- run-tests:
title: "wasm64_4gb"
test_targets: "wasm64_4gb"
Expand Down Expand Up @@ -772,6 +784,7 @@ jobs:
executor: bionic
environment:
EMTEST_SKIP_NODE_CANARY: "1"
EMTEST_SKIP_WASM64: "1"
Copy link
Member

Choose a reason for hiding this comment

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

is it necessary to have both this and the node canary update?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This builder does not have node canary installed. So it can no longer run wasm64 tests after this change. We could install node canary, but general we only configure one version of node per bot.

steps:
- run: apt-get install -q -y ninja-build scons
- run-tests-linux:
Expand All @@ -796,7 +809,14 @@ jobs:
steps:
- run-tests-chrome:
title: "browser64"
test_targets: "browser64"
# Skip tests that depend on running the output in node
# (This bot doesn't have node canary installed which is required to
# run wasm64 output).
test_targets: "browser64
skip:browser64.test_worker
skip:browser64.test_2gb_fail
skip:browser64.test_4gb_fail
skip:browser64.test_4gb"
test-browser-chrome-2gb:
executor: bionic
environment:
Expand Down Expand Up @@ -828,7 +848,15 @@ jobs:
# browser.test_webgl_offscreen_canvas_in_mainthread_after_pthread
# are crashing Firefox (bugzil.la/1281796). The former case is
# further blocked by issue #6897.
test_targets: "browser skip:browser.test_sdl2_mouse skip:browser.test_html5_webgl_create_context skip:browser.test_webgl_offscreen_canvas_in_pthread skip:browser.test_webgl_offscreen_canvas_in_mainthread_after_pthread skip:browser.test_glut_glutget"
test_targets: "
browser64.test_sdl_image
browser
skip:browser.test_sdl2_mouse
skip:browser.test_html5_webgl_create_context
skip:browser.test_webgl_offscreen_canvas_in_pthread
skip:browser.test_webgl_offscreen_canvas_in_mainthread_after_pthread
skip:browser.test_glut_glutget
"
# TODO(sbc): Re-enable once we figure out why the emrun tests are
# locking up.
#test-browser-chrome-emrun:
Expand Down
3 changes: 3 additions & 0 deletions src/library.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2394,6 +2394,9 @@ addToLibrary({
'initial': {{{ INITIAL_TABLE }}},
#if !ALLOW_TABLE_GROWTH
'maximum': {{{ INITIAL_TABLE }}},
#endif
#if MEMORY64 == 1
'index': 'i64',
#endif
'element': 'anyfunc'
});
Expand Down
16 changes: 11 additions & 5 deletions test/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,8 +872,8 @@ def node_is_canary(self, nodejs):
def require_node_canary(self):
nodejs = self.get_nodejs()
if self.node_is_canary(nodejs):
self.require_engine(nodejs)
return
self.require_engine(nodejs)
return

if 'EMTEST_SKIP_NODE_CANARY' in os.environ:
self.skipTest('test requires node canary and EMTEST_SKIP_NODE_CANARY is set')
Expand All @@ -889,10 +889,13 @@ def require_engine(self, engine):
self.wasm_engines = []

def require_wasm64(self):
if self.is_browser_test():
return

nodejs = self.get_nodejs()
if nodejs:
version = shared.get_node_version(nodejs)
if version >= (16, 0, 0):
if version >= (23, 0, 0):
self.js_engines = [nodejs]
self.node_args += shared.node_memory64_flags()
return
Expand All @@ -904,11 +907,14 @@ def require_wasm64(self):
return

if 'EMTEST_SKIP_WASM64' in os.environ:
self.skipTest('test requires node >= 16 or d8 (and EMTEST_SKIP_WASM64 is set)')
self.skipTest('test requires node >= 23 or d8 (and EMTEST_SKIP_WASM64 is set)')
else:
self.fail('either d8 or node >= 16 required to run wasm64 tests. Use EMTEST_SKIP_WASM64 to skip')
self.fail('either d8 or node >= 23 required to run wasm64 tests. Use EMTEST_SKIP_WASM64 to skip')

def require_simd(self):
if self.is_browser_test():
return

nodejs = self.get_nodejs()
if nodejs:
version = shared.get_node_version(nodejs)
Expand Down
2 changes: 1 addition & 1 deletion test/embind/imvu_test_adapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ function module(ignore, func) {
}

(function() {
var g = 'undefined' === typeof window ? global : window;
var g = 'undefined' === typeof window ? globalThis : window;

// synonyms
assert.equals = assert.equal;
Expand Down
1 change: 1 addition & 0 deletions test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3471,6 +3471,7 @@ def test_embind_tsgen_bigint(self):
self.run_process(args + ['-sWASM_BIGINT'])
self.assertFileContents(test_file('other/embind_tsgen_bigint.d.ts'), read_file('embind_tsgen_bigint.d.ts'))

@requires_wasm64
def test_embind_tsgen_memory64(self):
# Check that when memory64 is enabled longs & unsigned longs are mapped to bigint in the generated TS bindings
self.run_process([EMXX, test_file('other/embind_tsgen_memory64.cpp'),
Expand Down
9 changes: 9 additions & 0 deletions tools/feature_matrix.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class Feature(IntEnum):
THREADS = auto()
GLOBALTHIS = auto()
PROMISE_ANY = auto()
MEMORY64 = auto()


default_features = {Feature.SIGN_EXT, Feature.MUTABLE_GLOBALS}
Expand Down Expand Up @@ -82,6 +83,12 @@ class Feature(IntEnum):
'safari': 140000,
'node': 150000,
},
Feature.MEMORY64: {
'chrome': 128,
Copy link
Member

Choose a reason for hiding this comment

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

Is it premature to add this? we don't know which versions will ship this unflagged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anyone targeting memory64 is already seeing an experimental warning, so that we are really targetting here is some arbitrary interim version of memory64. Once we remove the experimental warning we will pin the final values here I imagine?

'firefox': 129,
'safari': UNSUPPORTED,
'node': 230000,
},
}


Expand Down Expand Up @@ -136,3 +143,5 @@ def apply_min_browser_versions():
enable_feature(Feature.BULK_MEMORY, 'pthreads')
if settings.AUDIO_WORKLET:
enable_feature(Feature.GLOBALTHIS, 'AUDIO_WORKLET')
if settings.MEMORY64 == 1:
enable_feature(Feature.MEMORY64, 'MEMORY64')
22 changes: 5 additions & 17 deletions tools/link.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,10 +393,7 @@ def check_human_readable_list(items):
passes += ['--pass-arg=asyncify-onlylist@%s' % ','.join(settings.ASYNCIFY_ONLY)]

if settings.MEMORY64 == 2:
passes += ['--memory64-lowering']

if settings.MEMORY64:
passes += ['--table64-lowering']
passes += ['--memory64-lowering', '--table64-lowering']
Copy link
Member

Choose a reason for hiding this comment

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

should --table64-lowering go away? I guess we can just roll the passes together into memory64-lowering now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, eventually, but I guess that can/should be a followup.


if settings.BINARYEN_IGNORE_IMPLICIT_TRAPS:
passes += ['--ignore-implicit-traps']
Expand Down Expand Up @@ -1368,17 +1365,6 @@ def phase_linker_setup(options, state, newargs):
settings.SUPPORTS_PROMISE_ANY = feature_matrix.caniuse(feature_matrix.Feature.PROMISE_ANY)
if not settings.BULK_MEMORY:
settings.BULK_MEMORY = feature_matrix.caniuse(feature_matrix.Feature.BULK_MEMORY)
if settings.BULK_MEMORY and settings.MEMORY64 and settings.MIN_NODE_VERSION < 180000:
# Note that we do not update tools/feature_matrix.py for this, as this issue is
# wasm64-specific: bulk memory for wasm32 has shipped in Node.js 12.5, but
# bulk memory for wasm64 has shipped only in Node.js 18.
#
# Feature matrix currently cannot express such complex combinations of
# features, so the only options are to either choose the least common
# denominator and disable bulk memory altogether for Node.js < 18 or to
# special-case this situation here. The former would be limiting for
# wasm32 users, so instead we do the latter:
settings.BULK_MEMORY = 0

if settings.AUDIO_WORKLET:
if settings.AUDIO_WORKLET == 1:
Expand Down Expand Up @@ -1977,9 +1963,11 @@ def run_embind_gen(wasm_target, js_syms, extra_settings, linker_inputs):
# Replace embind with the TypeScript generation version.
embind_index = settings.JS_LIBRARIES.index('embind/embind.js')
settings.JS_LIBRARIES[embind_index] = 'embind/embind_gen.js'
outfile_js = in_temp('tsgen_a.out.js')
if settings.MEMORY64:
settings.MIN_NODE_VERSION = 160000
outfile_js = in_temp('tsgen.js')
# The Wasm outfile may be modified by emscripten.emscript, so use a temporary file.
outfile_wasm = in_temp('tsgen_a.out.wasm')
outfile_wasm = in_temp('tsgen.wasm')
emscripten.emscript(wasm_target, outfile_wasm, outfile_js, js_syms, finalize=False)
# Build the flags needed by Node.js to properly run the output file.
node_args = []
Expand Down