In D125729#3883417, @sunfish wrote:

In D125729#3883414, @thakis wrote:

This might have broken check-lld: http://45.33.8.238/linux/89856/step_11.txt

Please take a look and revert for now if it takes a while to fix.

I'm looking into it now.

In D125729#3883414, @thakis wrote:

This might have broken check-lld: http://45.33.8.238/linux/89856/step_11.txt

Please take a look and revert for now if it takes a while to fix.

Looks good.

• Remove bulk-memory and nontrapping-fptoint.

• Add a comment about what "generic" means.

• Merge the comments for heap_base and heap_end.

In D70515#3857014, @sbc100 wrote:

Do you think this change is still useful and/or better than https://reviews.llvm.org/D135910?

• Address review feedback

In D135903#3856975, @sbc100 wrote:

• Fix indentation.

• Use a memory location instead of a wasm global, and support threads.
• Use llvm-objdump -d in tests.

In D135903#3856577, @sbc100 wrote:

Aren't reactors supposed to define an _initialize function that takes care of this?

LGTM!

In D133037#3762343, @sbc100 wrote:

The change to the code here seems only to relate to the module and the not the name/field of the import, is that right? Were fields already allowed to be empty? In which case maybe the title should just be "Allow empty module names in imports?"

Instead of adding new test files, add tests to existing test files.

In D131376#3737184, @sbc100 wrote:

I think its probably simpler to only allow a single start cabi_start definition.. just like any other symbol. It sounds like it should be a linker error to have more than one of them.

In curious why memorytExports is a set of strings rather than an optional string. Is there a need for exporting the memory with multiple names?

LGTM too. For testing, I think it would work to make a copy of llvm/test/CodeGen/WebAssembly/tls-local-exec.ll and change its thread_local(localexec)s to thread_locals.

In D81689#3614033, @yamt wrote:

In D81689#3613499, @sunfish wrote:

In D81689#3612529, @sbc100 wrote:

In D81689#3611504, @yamt wrote:

there are use cases for a command to export non-main functions.
eg. malloc/free stuff mentioned in https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/doc/memory_tune.md
this change broke them.

One consideration is that that wamr API doesn't seem compatible with closest thing we have to a spec, which states "Command instances may assume that they will be called from the environment at most once. Command instances may assume that none of their exports are accessed outside the duration of that call.". It's debatable how authoritative that spec is, but the big picture is that there's not currently much clarity on the relationship between LLVM and engines.

• if "outside the duration of that call" mean it's ok to access exports during the call of _start, the wamr api seems compatible to me. (those malloc/free exports are used during the execution of _start, as far as i understand.)

In D81689#3612529, @sbc100 wrote:

In D81689#3611504, @yamt wrote:

there are use cases for a command to export non-main functions.
eg. malloc/free stuff mentioned in https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/doc/memory_tune.md
this change broke them.

Update tests to use -mcpu=mvp to avoid breaking their target features when -mcpu=generic changes. And update target-features.ll to for the new -mcpu=generic features.

Add tests for -mcpu=mvp and -mcpu=bleeding-edge.

Add a driver test, and add release notes.

In D121327#3381123, @yln wrote:

@sunfish
Hi Dan, I hope you are still happy with this change. I didn't change any WebAssembly tests, but rather added a new IR-level test, so all existing WebAssembly behavior should stay the same. Let me know if you have any concerns.

The overall approach here makes sense to me. The tests test/CodeGen/WebAssembly/global_dtors.ll and test/CodeGen/WebAssembly/lower-global-dtors.ll cover the main interesting corner cases that we've hit with this pass; they currently have wasm targets and wasm-specific CHECK lines, but could likely be ported to other targets.

Looks good to me too!

Do we still intend to unify Emscripten's ABI with wasm32-unknown-unknown or wasm32-wasi eventually? This is talking a step away from that.

In D101271#2724145, @sbc100 wrote:

So you think it would be preferable to go with --export-if-defined then, or something wasm specific?

Looking at the ld documentation for export-dynamic-symbol:

I'm a little confused by the name "export-dynamic-symbol". I understand it's an ELF flag, though it seems to have a slightly different meaning here. What would you think about adding a warning when using this flag is -experimental-pic isn't passed, similar to what we do with -shared?

Great, thanks! And yes, switching to roundeven for both scalar and SIMD ISel sounds right to me.

This looks good to me! Could you briefly comment here on what the issue with llvm.roundeven is?

I don't see a way to do this with weak symbols, and an install script would be yet-another moving part that we'd have to make on end-user systems.

It's to ensure that older LLVM works with newer WASI libc, and newer clang works with older WASI libc. New-style commands require [lld support]. We can assume that if clang is updated, lld has the requisite support.

Ping!

Looks good!

Abandoned in favor of https://reviews.llvm.org/D89290 .

Either patch works for me. Yours is shorter :-).

LGTM; thanks for fixing this!

In D85062#2326618, @sbc100 wrote:

Fix (I believe) is in https://reviews.llvm.org/D89290

I've finished addressing the review comments, so this is now ready for review again.

Address review feedback.

Thanks! Description updated, fixes applied, and landed in https://reviews.llvm.org/rG6cd8511e5932