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

Friendly ping!

Ah, it seems the reason I didn't see this is that I wasn't using -flax-vector-conversions=none. Thanks for fixing this!

I've now opened https://github.com/WebAssembly/wasi-sdk/pull/151/ to remove the --no-threads from wasi-sdk, which was a workaround for an old bug that has long been fixed.

• Update clang/test/CodeGen/builtins-wasm.c.

• Reorganize the code a little so that we don't have to call mark multiple times.
• Fix a bug where we weren't considering calls from constructors as keeping other constructors live.
• Add a few more tests.

In D85062#2188643, @sbc100 wrote:

To be clear this change only relates to object files at are part of ar archives and are not part of the link? Perhaps mention that in the PR title/description.

Address review feedback.

• Rename -emscripten-pic to -experimental-pic
• Change the warning text to say "not stable" rather than "not yet implemented"

This change looks fine to me, though if you wanted to minimize the amount of code being emitted by the linker, another option would be to look into whether LLVM's FixFunctionBitcasts could handle this -- there should be a bitcast in the llvm.global_ctors array.

Landed in 66042959590d6db9d2a12803a16476d4e3508f3f.

• Address review feedback
• Mark callCtors live in the MarkLive pass rather than in the Writer pass.

Run clang-format and fix lint warnings.

Use the return value of handleUndefined instead of doing a separate lookup, which avoids a redundant lookup and handles LazySymbols properly.

I apologize again for the major delay. I've now updated the patch and addressed all of your comments.

• Add tests for redeclaration behavior
• Remove disabled tests (previously marked with FIXMEs)
• Made the mismatch warning more informative.

Looks good to me.

In D70500#1999268, @bernhard wrote:

It's for users who want smaller wasm binaries. It's not currently documented, though yes, it would be nice to document it.

But how would a user even end up with wasm-opt in the same directory of clang binaries?

In D70500#1998994, @bernhard wrote:

Are there plans to offer a way to disable this behavior (or have it optional in the first place)?
We'd like to run some custom processing between wasm-ld and wasm-opt which can't happen after the latter due to some of its one-way destructive optimizations (i.e. memory-packing or simplify-globals passes).
The only way now is to tell our users to place wasm-opt somewhere where clang can't find it. Or instead of using one clang super-command to manually call -cc1 and wasm-ld separately which is disappointing.

Also, is it even common to place wasm-opt next to the clang executable? Who is this for? Is this documented?

This addresses the review feedback from earlier. To answer this question:

Rebase, update, add a test, and add basic error reporting.

Looks good! I don't have anything to add beyond Thomas' review comments.

Is looks possible that the test failure was due to an unrelated commit 210f40fe9a30212396311d265904b2d73859c53d. If so, it's possible it's fixed as of 74ab5d98d07f0b0226f45ccca8df6a450d52fb7b.

On some 32-bit x86 hosts, particularly those that use x87 arithmetic, loading and storing with floating-point types can change the NaN bit pattern. To preserve the bit pattern reliably, MCOperand should represent floating-point immediates by representing the bits in int32_t for 32-bit and int64_t for 64-bit. Codegen and MC don't usually look at the value, but where they do need to interpret it as a floating-point value, they can reinterpret the bits on demand.

Cool, LGTM, with optional suggestion for signed char below:

Very cool, thanks for putting this together!