Skip to content
This repository was archived by the owner on Jan 15, 2025. It is now read-only.

LLVM: wasm64, atomics (exception handling) not really implemented #12

Closed
ecs-deutschland-gmbh opened this issue Nov 30, 2020 · 14 comments
Closed

Comments

@ecs-deutschland-gmbh
Copy link

Hi,

is anyone aware whether a working implementation of atomics instr. with target=wasm64 can be expected soon?
EH depends on it and some more.

Just many thanks for any information regarding this. I could not quickly find a better place to ask.

FUNC __wasm_init_memory with target=wasm64:
i32.const(67616) // of course NOT ;)
i32.const(0)
i32.const(1)
i32.atomic.rmw.cmpxchg(0 align 2)
if.op( extdatab @112)b1_len=11 b2_len=27 b1_stream_start=<4> b2_stream_start=11 stream_end=<26> depth=1 parent@0
i32.const(67616)
i32.const(1)
i64.const(-1)
i32.atomic.wait(0 align 2)
drop()
else.op()
i32.const(65536)
i32.const(0)
i32.const(8)
memory.init( extdatab @48)
i32.const(65544)
i32.const(0)
i32.const(16)
memory.init( extdatab @80)
i32.const(67616)
i32.const(2)
i32.atomic.store(0 align 2)
i32.const(67616)
i32.const(-1)
atomic.notify(0 align 2)
drop()
end()
data.drop()
data.drop()

@ecs-deutschland-gmbh
Copy link
Author

I just see this is related to void Writer::createInitMemoryFunction(), in wasm_ld. If anyone knows if/when this could theoretically get 'fixed', thanks for any info :;

@tlively
Copy link
Member

tlively commented Nov 30, 2020

cc @aardappel. Would you be able to update createInitMemoryFunction? Let me know if you have any questions about it.

@aardappel
Copy link
Contributor

@tlively sure, I can have a look.

@ecs-commonA note that while wasm64 support in LLVM is intended to be fairly complete, it has not been tested extensively beyond the basic tests in LLVM (and now WABT, Binaryen etc). It still needs "end to end" toolchain tests of larger projects (which I am currently working on for Emscripten), and we don't even have an engine that can run it yet.

@aardappel
Copy link
Contributor

Possible fix: https://reviews.llvm.org/D92348

@ecs-deutschland-gmbh
Copy link
Author

Thanks to you guys for reacting so quickly and taking care of this. I can confirm wasm64 in LLVM is REALLY nice already (thx Wouter), exception handling does yet appear to have problems:

Compiling with -fwasm-exceptions and throwing/catching some works fine on wasm32 yet, but on wasm64 there is

fatal error: error in backend: Cannot select: intrinsic %llvm.wasm.extract.exception
clang: error: clang frontend command failed with exit code 70 (use -v to see invocation)
Ubuntu clang version 12.0.0-++20201103081716+a44b7322a27-1exp120201103072359.373
Target: wasm64
Thread model: posix
InstalledDir: /usr/bin
clang: note: diagnostic msg:


PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang: note: diagnostic msg: /tmp/6-3b31a5.cpp
clang: note: diagnostic msg: /tmp/6-3b31a5.sh
clang: note: diagnostic msg:


@ecs-deutschland-gmbh
Copy link
Author

... I see, for wasm64 and EH to work also the $__wasm_init_tls func would need to be updated. This function also addresses a global (1) that most likely would need to be changed from i32 to i64 I think....

@dschuff
Copy link
Member

dschuff commented Dec 1, 2020

-fwasm-exceptions should not be expected to work right now, as we are updating it (both in the toolchain and V8) to reflect changes in the wasm EH spec.

@ecs-deutschland-gmbh
Copy link
Author

Hi,

re: Possible fix: https://reviews.llvm.org/D92348

I see the fix has landed but it appears incomplete to me, I think all instructions marked with '+' below shall be i64.const.
The args for memory.init must all be i64 IMO (addr, start, size, all are 'memarg'). Please correct me if wrong.

+i32.const(67616)
i32.const(0)
i32.const(1)
i32.atomic.rmw.cmpxchg(0 align 2)
if.op( extdatab @112)b1_len=11 b2_len=27 b1_stream_start=<4> b2_stream_start=11 stream_end=<26> depth=1 parent@0

  • i32.const(67616)
    i32.const(1)
    i64.const(-1)
    i32.atomic.wait(0 align 2)
    drop()
    else.op()
    i64.const(65536)
  • i32.const(0)
  • i32.const(8)
    memory.init( extdatab @48)
    i64.const(65544)
  • i32.const(0)
  • i32.const(16)
    memory.init( extdatab @80)
  • i32.const(67616)
    i32.const(2)
    i32.atomic.store(0 align 2)
  • i32.const(67616)
    i32.const(-1)
    atomic.notify(0 align 2)
    drop()
    end()
    data.drop()
    data.drop()
    end.func()

Regarding the new EH proposal (#3) I'm bit sad/disappointed, because I implemented #2 and found it's nice (LLVM win EH based, thx to Heejin). I really hope, #3 will be close to #2, at least for LLVM.

@dschuff
Copy link
Member

dschuff commented Dec 2, 2020

+cc @aheejin
If by "at least for LLVM" you mean will the LLVM IR be the same (i.e. using win EH), then I believe the answer is yes. It will just be translated to wasm differently.

@aheejin
Copy link
Member

aheejin commented Dec 2, 2020

As @dschuff said, we are implementing recent changes (WebAssembly/exception-handling#137 and WebAssembly/exception-handling#143) in the spec, so please don't expect to work it in both 32 and 64 bits some months. The existing support was not tested for wasm64, but I think extending the support to wasm64 after finishing the new spec implementation shouldn't be hard.

@aardappel
Copy link
Contributor

@ecs-commonA the patch has not been merged yet, and as you can see here https://github.com/WebAssembly/memory64/blob/master/proposals/memory64/Overview.md the memory.init only takes a single i64 arg.

@ecs-deutschland-gmbh
Copy link
Author

Aardappel, thank you. I did not yet see this. I can confirm the wasm64 support is very good yet, even wasi-libc > dlmalloc etc. work perfectly after I removed some static asserts.

@aardappel
Copy link
Contributor

@ecs-commonA https://reviews.llvm.org/D92348 has been merged, please let us know if its working for you once you get the chance.

@ecs-deutschland-gmbh
Copy link
Author

Thanks Aardappel. Confirmed, function looks fine :)

sbc100 pushed a commit that referenced this issue Aug 1, 2024
* Fix binary grammar definition of the branch hints custom section

The overall section structure definition wasm missing.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants