Add initial support for using lld rather than s2wasm in the wasm backend #5313

sbc100 · Jun 15, 2017

This path is currently experimental and is activated by setting EMCC_WASM_BACKEND=2.

This change does 3 things:

Support for setting WASM_BACKEND=2, and checking lld version support
Support for building wasm_compiler_rt libararies as .o archives in addition to current .s archives
Support for using lld to produce the final wasm file, rather than s2wasm + wasm-as.

kripken · Jun 15, 2017

kripken reviewed Jun 15, 2017

View changes

kripken · Jun 15, 2017

emcc.py

@@ -479,8 +480,9 @@ def filter_emscripten_options(argv):
                                               # but then, we don't care about bitcode outputs anyhow, below, so
                                               # skipping to exit(ret) is fine
      if target.endswith('.js'):
-        shutil.copyfile(target, target[:-3])
-        target = target[:-3]
+        bare_target = os.path.splitext(target)[0]


we have a utility for that, unsuffixed

kripken · Jun 15, 2017

kripken reviewed Jun 15, 2017

View changes

kripken · Jun 15, 2017

emcc.py

@@ -1444,10 +1454,11 @@ def get_final():
    with ToolchainProfiler.profile_block('post-link'):
      if DEBUG:
        logging.debug('saving intermediate processing steps to %s', shared.get_emscripten_temp_dir())
-        if not LEAVE_INPUTS_RAW: save_intermediate('basebc', 'bc')
+        if not LEAVE_INPUTS_RAW and not shared.Settings.WASM_BACKEND:
+          save_intermediate('basebc', 'bc')


shouldn't this be if using obj files?

kripken · Jun 15, 2017

kripken reviewed Jun 15, 2017

View changes

kripken · Jun 15, 2017

emcc.py


      # Optimize, if asked to
-      if not LEAVE_INPUTS_RAW:
+      if not LEAVE_INPUTS_RAW and not shared.Settings.WASM_BACKEND:


ditto, looks like this would disable lto for the wasm backend (when not using wasm object files)

kripken · Jun 15, 2017

kripken reviewed Jun 15, 2017

View changes

kripken · Jun 15, 2017

emcc.py

@@ -1525,10 +1536,14 @@ def get_final():

      if shared.Settings.WASM_BACKEND:
        # we also received wast and wasm at this stage
-        temp_basename = final[:-3]
+        temp_basename = os.path.splitext(final)[0]


unsuffixed

kripken · Jun 15, 2017

kripken reviewed Jun 15, 2017

View changes

kripken · Jun 15, 2017

emscripten.py

@@ -395,9 +384,9 @@ def create_module(function_table_sigs, metadata, settings,


 def write_output_file(outfile, post, module):
-  for i in range(len(module)): # do this loop carefully to save memory


i think this loop intentionally did not leave a variable with chunk contents alive. here, the new variable chunk survives. maybe this mattered a long time ago but not now, though, but we should verify

kripken · Jun 15, 2017

kripken reviewed Jun 15, 2017

View changes

kripken · Jun 15, 2017

tools/shared.py

@@ -333,7 +331,10 @@ def get_clang_version():
  return actual_clang_version

 def check_clang_version():
-  expected = '.'.join(map(str, EXPECTED_LLVM_VERSION))


is EXPECTED_LLVM_VERSION not used anywhere else? i think it might be, maybe in the tests? I could be wrong.

kripken · Jun 15, 2017

kripken reviewed Jun 15, 2017

View changes

kripken · Jun 15, 2017

tools/shared.py

@@ -903,14 +905,18 @@ def set_logging():

 # Target choice.
 ASM_JS_TARGET = 'asmjs-unknown-emscripten'
-WASM_TARGET = 'wasm32-unknown-unknown'
+WASM_TARGET = 'wasm32-unknown-unknown-elf'
+WASM_OBJ_TARGET = 'wasm32-unknown-unknown-wasm'



this surprises me - a different triple just for the feature of using wasm object files?

kripken · Jun 15, 2017

kripken reviewed Jun 15, 2017

View changes

kripken · Jun 15, 2017

tools/system_libs.py

-    return in_temp(lib_filename)
+    output = in_temp(lib_filename)
+    if shared.get_llvm_target() == shared.WASM_OBJ_TARGET:
+      shared.Building.emar('cr', output, o_s)


they could be linked using lld, instead of an archive? we specifically only use an archive for libc++ i believe, and this would change that. however, I don't remember all the details of why we do that atm.

kripken · Jun 15, 2017

kripken reviewed Jun 15, 2017

View changes

kripken · Jun 15, 2017

tools/shared.py

+        elif not Building.is_ar(absolute_path_f):
+          if get_llvm_target() == WASM_OBJ_TARGET:
+            if Building.is_wasm(f):
+              actual_files.append(f)


no else? perhaps it should throw an error?

sbc100 · Jul 12, 2017

This change is not much more focused. PTAL.

jgravelle-google · Jul 12, 2017

jgravelle-google reviewed Jul 12, 2017

View changes

jgravelle-google · Jul 12, 2017

tools/shared.py

@@ -398,6 +398,12 @@ def check_fastcomp():
        print >> sys.stderr, '==========================================================================='
        return False

+    if Settings.WASM_BACKEND == 2:
+      if subprocess.call([LLVM_LLD, '-flavor', 'wasm', '--version'], stdout=PIPE, stderr=PIPE) != 0:
+        logging.critical('WASM_BACKEND=2 but lld not build with wasm support:')


Typo: build -> built

jgravelle-google · Jul 12, 2017

emscripten.py

+  compiler_rt_lib = shared.Cache.get('libwasm_compiler_rt.a', wasm_rt_fail('libwasm_compiler_rt.a'), 'a')
+  libc_rt_lib = shared.Cache.get('libwasm_libc_rt.a', wasm_rt_fail('libwasm_libc_rt.a'), 'a')
+
+  args = [shared.LLVM_LLD, '-flavor', 'wasm', temp_o] #, '--strip-debug']


Remove commented out code

jgravelle-google · Jul 12, 2017

emscripten.py

+
+
+def create_lld_args(temp_o, output):
+  def wasm_rt_fail(archive_file):


Might as well combine this with the one in create_s2wasm_args and promote them to top-level functions

jgravelle-google · Jul 12, 2017

emscripten.py

+  return metadata
+
+
+def archive_intermediate(filename, output_name):


Yay helper functions

jgravelle-google · Jul 12, 2017

emscripten.py


+  import pprint


Imports should be at the top of the file

jgravelle-google · Jul 12, 2017

emscripten.py

+    wast = basename + '.wast'
+    build_wasm_lld(temp_files, infile, wasm_file, settings, DEBUG)
+    wasm_dis(wasm_file, wast)
+    sending = []


Why initialize these here?

jgravelle-google · Jul 12, 2017

emscripten.py

+  if shared.Settings.WASM_BACKEND == 2:
+    basename = shared.unsuffixed(outfile.name)
+    wasm_file = basename + '.wasm'
+    wast = basename + '.wast'


I feel like this should be wast_file to match wasm_file

kripken · Jul 12, 2017

kripken reviewed Jul 12, 2017

View changes

Please document the meaning of Settings.WASM_BACKEND mode 2, in src/settings.js

kripken · Jul 12, 2017

emscripten.py

-def build_wasm(temp_files, infile, outfile, settings, DEBUG):
-  with temp_files.get_file('.wb.s') as temp_s:
-    backend_args = create_backend_args_wasm(infile, temp_s, settings)
+def wasm_dis(wasm_file, wast_file):


this should probably be in tools/shared.py, we have a bunch of utility functions there like llvm_opt etc. that call the various llvm tools, so this could fit alongside them

kripken · Jul 12, 2017

tools/system_libs.py

-    shared.Cache.get('wasm_compiler_rt.a', lambda: create_wasm_compiler_rt('wasm_compiler_rt.a'), extension='a')
-    shared.Cache.get('wasm_libc_rt.a', lambda: create_wasm_libc_rt('wasm_libc_rt.a'), extension='a')
+    if shared.Settings.WASM_BACKEND == 2:
+      shared.Cache.get('libwasm_compiler_rt.a', lambda: create_wasm_compiler_rt('libwasm_compiler_rt.a'), extension='a')


is this just because the lib prefix is necessary for lld? if the contents are identical, could we just always call it the lib* name?

sbc100 requested review from dschuff and jgravelle-google Jun 15, 2017

sbc100 force-pushed the lld branch 3 times, most recently from d745c53 to af6fdd5 Jun 21, 2017

sbc100 force-pushed the lld branch 3 times, most recently from 1c2cfb6 to 78ac896 Jul 7, 2017

sbc100 changed the title ~~WIP: Add a wasm backend mode that uses lld rather than s2wasm~~ Add initial support for using lld rather than s2wasm in the wasm backend Jul 12, 2017

sbc100 added some commits Jun 28, 2017

Add initial support for linking with lld rather than s2wasm

236907e

Merge remote-tracking branch 'origin/incoming' into lld

Loading status checks…

7ad6459

sbc100 force-pushed the lld branch from 991b4f6 to 7ad6459 Nov 21, 2017

jgravelle-google referenced this pull request Jan 10, 2018
Merged
First-pass at LLD support behind EXPERIMENTAL_USE_LLD flag #6056

tbfleming referenced this pull request Feb 20, 2018
Open
Mismatched calling convention linking to function returning struct #6259

juj force-pushed the incoming branch from e1622af to d54eb4e May 21, 2018

sbc100 closed this Jul 2, 2018

sbc100 deleted the lld branch Jul 2, 2018

emscripten-core/emscripten

Join GitHub today

Add initial support for using lld rather than s2wasm in the wasm backend #5313

Conversation

sbc100 commented Jun 15, 2017 • edited

sbc100 requested review from dschuff and jgravelle-google Jun 15, 2017

kripken reviewed Jun 15, 2017 View changes

This comment has been minimized.

kripken reviewed Jun 15, 2017 View changes

This comment has been minimized.

kripken reviewed Jun 15, 2017 View changes

This comment has been minimized.

kripken reviewed Jun 15, 2017 View changes

This comment has been minimized.

kripken reviewed Jun 15, 2017 View changes

This comment has been minimized.

kripken reviewed Jun 15, 2017 View changes

This comment has been minimized.

kripken reviewed Jun 15, 2017 View changes

This comment has been minimized.

kripken reviewed Jun 15, 2017 View changes

This comment has been minimized.

kripken reviewed Jun 15, 2017 View changes

This comment has been minimized.

sbc100 force-pushed the lld branch 3 times, most recently from d745c53 to af6fdd5 Jun 21, 2017

sbc100 force-pushed the lld branch 3 times, most recently from 1c2cfb6 to 78ac896 Jul 7, 2017

sbc100 changed the title WIP: Add a wasm backend mode that uses lld rather than s2wasm Add initial support for using lld rather than s2wasm in the wasm backend Jul 12, 2017

This comment has been minimized.

sbc100 commented Jul 12, 2017

jgravelle-google reviewed Jul 12, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kripken reviewed Jul 12, 2017 View changes

kripken left a comment

This comment has been minimized.

This comment has been minimized.

sbc100 added some commits Jun 28, 2017

sbc100 force-pushed the lld branch from 991b4f6 to 7ad6459 Nov 21, 2017

jgravelle-google referenced this pull request Jan 10, 2018

tbfleming referenced this pull request Feb 20, 2018

juj force-pushed the incoming branch from e1622af to d54eb4e May 21, 2018

sbc100 closed this Jul 2, 2018

sbc100 deleted the lld branch Jul 2, 2018

sbc100 commented Jun 15, 2017 •

edited

kripken reviewed Jun 15, 2017

View changes

kripken reviewed Jun 15, 2017

View changes

kripken reviewed Jun 15, 2017

View changes

kripken reviewed Jun 15, 2017

View changes

kripken reviewed Jun 15, 2017

View changes

kripken reviewed Jun 15, 2017

View changes

kripken reviewed Jun 15, 2017

View changes

kripken reviewed Jun 15, 2017

View changes

kripken reviewed Jun 15, 2017

View changes

sbc100 force-pushed the lld branch 3 times, most recently from `d745c53` to `af6fdd5` Jun 21, 2017

sbc100 force-pushed the lld branch 3 times, most recently from `1c2cfb6` to `78ac896` Jul 7, 2017

jgravelle-google reviewed Jul 12, 2017

View changes

kripken reviewed Jul 12, 2017

View changes

sbc100 force-pushed the lld branch from `991b4f6` to `7ad6459` Nov 21, 2017

juj force-pushed the incoming branch from `e1622af` to `d54eb4e` May 21, 2018