Skip to content

[wasm64] making JS bindings wasm64 aware #12869

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
Oct 15, 2021
Merged

[wasm64] making JS bindings wasm64 aware #12869

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
10 changes: 7 additions & 3 deletions src/runtime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ var Compiletime = {
// code used both at compile time and runtime is defined here, then put on
// the Runtime object for compile time and support.js for the generated code

function getPointerSize() {
return MEMORY64 ? 8 : 4;
}

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 can be removed and just use POINTER_SIZE.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there is some funny scope / dependency / ordering issue between these files last time I tried to sort this out, and I seem to remember POINTER_SIZE not being available everywhere. Below we have POINTER_SIZE: getPointerSize() and turning that into POINTER_SIZE: POINTER_SIZE will most certainly not work.

The double definition of getPointerSize was also necessary, we even had some discussion on that, forget where.

I'd love to hear more specific ideas on how to un-knot this.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like parseTools is executed first in compiler.js. So we could define POINTER_SIZE there and use it here. I think we already define it there, so maybe after recent changes it will just work?

If not, what error do you get?

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 could go figure that all out, but I'd rather do this in a follow-up.

function getNativeTypeSize(type) {
switch (type) {
case 'i1': case 'i8': return 1;
Expand All @@ -31,7 +35,7 @@ function getNativeTypeSize(type) {
case 'double': return 8;
default: {
if (type[type.length-1] === '*') {
return 4; // A pointer
return getPointerSize();
} else if (type[0] === 'i') {
var bits = Number(type.substr(1));
assert(bits % 8 === 0, 'getNativeTypeSize invalid bits ' + bits + ', type ' + type);
Expand All @@ -53,6 +57,6 @@ var Runtime = {
return Math.max(getNativeTypeSize(type), Runtime.QUANTUM_SIZE);
},

POINTER_SIZE: 4,
QUANTUM_SIZE: 4,
POINTER_SIZE: getPointerSize(),
QUANTUM_SIZE: getPointerSize(),
};
4 changes: 4 additions & 0 deletions src/support.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

var STACK_ALIGN = {{{ STACK_ALIGN }}};

function getPointerSize() {
return {{{ MEMORY64 ? 8 : 4 }}};
}
Copy link
Member

Choose a reason for hiding this comment

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

Can remove this too, and use POINTER_SIZE.


{{{ getNativeTypeSize }}}

function warnOnce(text) {
Expand Down
7 changes: 4 additions & 3 deletions tests/core/test_typeid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,18 @@
#include <stdio.h>
#include <string.h>
#include <typeinfo>
#include <stdint.h>
int main() {
printf("*\n");
#define MAX 100
int ptrs[MAX];
long ptrs[MAX];
int groups[MAX];
memset(ptrs, 0, MAX * sizeof(int));
memset(ptrs, 0, MAX * sizeof(long));
memset(groups, 0, MAX * sizeof(int));
int next_group = 1;
#define TEST(X) \
{ \
int ptr = (int)&typeid(X); \
intptr_t ptr = (intptr_t)&typeid(X); \
int group = 0; \
int i; \
for (i = 0; i < MAX; i++) { \
Expand Down
6 changes: 6 additions & 0 deletions tests/runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
'wasm2js3',
'wasm2jss',
'wasm2jsz',
'wasm64'
]

# The default core test mode, used when none is specified
Expand Down Expand Up @@ -312,6 +313,8 @@ def parse_args(args):
parser.add_argument('tests', nargs='*')
parser.add_argument('--failfast', dest='failfast', action='store_const',
const=True, default=False)
parser.add_argument('--force64', dest='force64', action='store_const',
const=True, default=False)
return parser.parse_args()


Expand All @@ -324,6 +327,8 @@ def configure():
common.EMTEST_LACKS_NATIVE_CLANG = int(os.getenv('EMTEST_LACKS_NATIVE_CLANG', '0'))
common.EMTEST_REBASELINE = int(os.getenv('EMTEST_REBASELINE', '0'))
common.EMTEST_VERBOSE = int(os.getenv('EMTEST_VERBOSE', '0')) or shared.DEBUG
global FORCE64
FORCE64 = int(os.getenv('EMTEST_FORCE64', '0'))
if common.EMTEST_VERBOSE:
logging.root.setLevel(logging.DEBUG)

Expand Down Expand Up @@ -361,6 +366,7 @@ def set_env(name, option_value):
set_env('EMTEST_REBASELINE', options.rebaseline)
set_env('EMTEST_VERBOSE', options.verbose)
set_env('EMTEST_CORES', options.cores)
set_env('EMTEST_FORCE64', options.force64)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add FORCE64 = int(os.getenv('EMTEST_FORCE64', '0')) above in configure and then remove FORCE64 from tests/test_benchmark.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That didn't work, had keep at least FORCE64 = 0 in the other file and add global FORCE64 here..?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant common.FORCE64 = int(os.getenv('EMTEST_FORCE64', '0')) (with the common prefix).

And then in common.py you would do EMTEST_FORCE64 = 0 and then in tests/test_benchmark.py you would do import common and access it as common.EMTEST_FORCE64. That is the pattern that the other options use anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This still needs to be resolved.. we shouldn't need the global keyword above.


configure()

Expand Down
45 changes: 31 additions & 14 deletions tests/test_benchmark.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@

LLVM_FEATURE_FLAGS = ['-mnontrapping-fptoint']

FORCE64 = 0


class Benchmarker():
# called when we init the object, which is during startup, even if we are
Expand Down Expand Up @@ -205,12 +207,14 @@ def build(self, parent, filename, args, shared_args, emcc_args, native_args, nat
OPTIMIZATIONS,
'-s', 'INITIAL_MEMORY=256MB',
'-s', 'FILESYSTEM=0',
'--closure=1',
'-s', 'MINIMAL_RUNTIME',
'-s', 'ENVIRONMENT=node,shell',
'-s', 'BENCHMARK=%d' % (1 if IGNORE_COMPILATION and not has_output_parser else 0),
'-o', final
] + shared_args + emcc_args + LLVM_FEATURE_FLAGS + self.extra_args
if FORCE64:
cmd += ['--profiling']
else:
cmd += ['--closure=1', '-sMINIMAL_RUNTIME']
if 'FORCE_FILESYSTEM' in cmd:
cmd = [arg if arg != 'FILESYSTEM=0' else 'FILESYSTEM=1' for arg in cmd]
if PROFILING:
Expand Down Expand Up @@ -353,22 +357,30 @@ def cleanup(self):

# Benchmarkers

benchmarkers = [
NativeBenchmarker('clang', [CLANG_CC], [CLANG_CXX]),
# NativeBenchmarker('gcc', ['gcc', '-no-pie'], ['g++', '-no-pie'])
]
benchmarkers = []

if not FORCE64:
benchmarkers += [
NativeBenchmarker('clang', [CLANG_CC], [CLANG_CXX]),
# NativeBenchmarker('gcc', ['gcc', '-no-pie'], ['g++', '-no-pie'])
]

if config.V8_ENGINE and config.V8_ENGINE in config.JS_ENGINES:
# avoid the baseline compiler running, because it adds a lot of noise
# (the nondeterministic time it takes to get to the full compiler ends up
# mattering as much as the actual benchmark)
aot_v8 = config.V8_ENGINE + ['--no-liftoff']
default_v8_name = os.environ.get('EMBENCH_NAME') or 'v8'
benchmarkers += [
EmscriptenBenchmarker(default_v8_name, aot_v8),
EmscriptenBenchmarker(default_v8_name + '-lto', aot_v8, ['-flto']),
# EmscriptenWasm2CBenchmarker('wasm2c')
]
if FORCE64:
benchmarkers += [
EmscriptenBenchmarker(default_v8_name, aot_v8, ['-sMEMORY64=2']),
]
else:
benchmarkers += [
EmscriptenBenchmarker(default_v8_name, aot_v8),
EmscriptenBenchmarker(default_v8_name + '-lto', aot_v8, ['-flto']),
# EmscriptenWasm2CBenchmarker('wasm2c')
]
if os.path.exists(CHEERP_BIN):
benchmarkers += [
# CheerpBenchmarker('cheerp-v8-wasm', aot_v8),
Expand All @@ -385,9 +397,14 @@ def cleanup(self):
]

if config.NODE_JS and config.NODE_JS in config.JS_ENGINES:
benchmarkers += [
# EmscriptenBenchmarker('Node.js', config.NODE_JS),
]
if FORCE64:
benchmarkers += [
EmscriptenBenchmarker('Node.js', config.NODE_JS, ['-sMEMORY64=2']),
]
else:
benchmarkers += [
# EmscriptenBenchmarker('Node.js', config.NODE_JS),
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about the changes to this file.. My understanding is that folks are expect to edit it to their needs so checking in this one configuration seems odd/wrong. Perhaps remove this file from this PR since its not needed for CI.. its more a local testing thing for you I guess?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That seem like a lot of changes that I'd have to re-do / re-figure-out each time I run this, I don't understand why I wouldn't want this runnable out of the box? where do I "store" these modifications when I am not using them? why not have some good defaults that are useful to people?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Normally what we do is create a new suite in test_core.py when we want to get good coverage for a new features.. and that has historically been enough. I see you have already done that for wasm64 which is great.

This change seems like a new way to configure the benchmark suite which so far we don't have. I'm just not sure why we would single out wasm64 and hardcode all these settings like this when we don't do it for other features such as LTO and ASan.

I guess its fine.. I basically never run this suite so I don't feel strongly.. what do you think @kripken ?

Copy link
Member

Choose a reason for hiding this comment

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

Wasm64 does seem special enough to justify this, I think. And we do have a few other global params here like profiling etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kripken WDYT about these changes to test_benchmark.py

Copy link
Member

Choose a reason for hiding this comment

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

+1 from me for these changes to benchmarking.



class benchmark(common.RunnerCore):
Expand Down
Loading