• -matomics means -mthread-model posix

That sounds reasonable to me too.

In D57632#1384200, @sbc100 wrote:

Looking good.

Can we can a test in the MC/WebAssembly and lld/test/wasm?

• Add MC/WebAssembly and lld tests
• Set the WASM_SYMBOL_NAME_EXPLICIT flag when a symbols name differs from its import name.

• Rename printf_no_Lf to __small_printf.
• Add more tests.
• Split out enabling the iprintf optimization for WASI, to be submitted in a separate patch later.

• Use a symbol flag to indicate when a custom symbol name is present.
• Revert the version number bump.
• Allow globals and events to have custom symbol names too, for consistency.

In D57602#1381220, @sbc100 wrote:

Do you could also achieve this I guess by renaming pwsix.read function itself to pwsix.__read? But I guess it makes sense to allow the use to control all 3 parts of the name separately.

I think WebAssembly is in the same situation as most other architectures, as discussed here:

In D57323#1374682, @sbc100 wrote:

• Remove the target constraint, allowing the main rewrite logic to work on all wasm32 targets.
• Remove the now-unneeded "wasm-temporary-workarounds" flag altogether.
• This uncovered some bugs with wrappers for main when main is only a declaration; fix those as well.

In D57323#1374572, @sbc100 wrote:

I mean with your fix we don't need this to be wasi specific do we?

In D57323#1374325, @sbc100 wrote:

I tried to enable this by default recently but had to revert: https://reviews.llvm.org/D54117.

I'm fine for this to land in the mean time but you might want to be aware of this issue.

LGTM.

Sorry for the delay here. Yes, let's land this.

In D56553#1356720, @sbc100 wrote:

Can you make change to llvm and clang at the same time? I thought that was not possible..

Title updated.

Or, here's another idea: What if we *just* add "include/wasm-$OS" and don't add "wasm32-$OS" or "wasm64-$OS"-specific include directories? Most headers for wasm will probably be shared between the two, and for anything that really can't be, we always have the __wasm32__ and __wasm64__ predefined macros.

In D56553#1356482, @dschuff wrote:

One difference between the multilib style and multi-arch style is whether any headers can be shared between the 2 arches. Historically at least, with bitness-flavored variants of the same architecture there were common headers used by both, and it was just the library directories that needed to be different. I've heard anecdotally that this is not the case for ARM though.

In D56553#1356467, @sbc100 wrote:

Ooo - a new OS is born!

Per @sbc100's comment, this update switches to full multilib paths.

With https://reviews.llvm.org/D48471 and related work, it's not less urgent to do this. And since there were objections to having target-specific warnings anyway, let's close this.

@aheejin The issue is that the current code doesn't distinguish between reading and writing of the stack pointer. Calls don't effectively modify the stack pointer, but they do read the stack pointer. Instructions which modify the stack pointer shouldn't be reordered past calls.

This patch has a review. Is it ready to land?

As @nw points out, clang's lib/Headers/stdint.h doesn't support this yet. Unless anyone feels strongly, I now propose we close this and just leave int_fast16_t/uint_fast16_t as-is.

From a survey and some testing, Emscripten doesn't appear to rely on these paths, so this just affects things like wasmception and the waterfall, which should be straightforward to update.

In D55467#1326244, @kripken wrote:

In D55467#1326177, @sunfish wrote:

Do we know what part of the optimizer or codegen is introducing the irreducible control flow in malloc?

Not for malloc - I think @jgravelle-google may have been looking into that though?

Do we know what part of the optimizer or codegen is introducing the irreducible control flow in malloc?

-mnan and --with-nan= are MIPS target options, however I'm asking about targeting WebAssembly with a MIPS host.

If I write C code like this:

Can we remove the optional altogether now, and just always do the fixup?

LGTM. In the future it's possible we could need to hold the signatures somewhere other than the AsmPrinter, however we can figure that out later, when it's more clear what the MC AsmParser layer will need.

In D52450#1248681, @tlively wrote:

That's true, except that if the semantics of the LLVM intrinsics were not as strict as the semantics of the WebAssembly instructions, then there would be no way for programmers to access WebAssembly's exact semantics at the source level, which is the whole point of implementing these intrinsics at all.

In D52450#1248586, @tlively wrote:

WebAssembly's handling of +0/-0 *does* match IEEE 754-2018, so these intrinsics (whether target-independent or not) need to be changed to match those semantics as well. I will have to investigate whether the fminnan and fmaxnan ISel DAG nodes are already being used in ways that do not respect these semantics. If they are, we will need new ISel DAG nodes as well.

In D52372#1244158, @aheejin wrote:

1. Is it correct to translate those llvm instructions to saturating versions of wasm instructions? For example, for scalars, we had a patch that implemented the saturating behavior. Don't we need a similar thing for vectors too?

The prompt here is that the LLVM wasm backend will be leaving "experimental" status. That means in about 6 months, it'll be part of an LLVM release. Once that happens, it'll be harder to make changes like this.

In D43211#1241361, @sbc100 wrote:

Woah! This a big step people! Congratulations.

@sbc100 @ncw Can you comment on the status of this issue? Is this still something we want to fix before we consider the basic ABI to be basically stable?

https://reviews.llvm.org/D50568 is now fixed!

On one hand, clang-format -style=LLVM really isn't the one exclusive formatting style of LLVM. CodingStandards.rst repeatedly emphasizes this. When we're working in LLVM outside of wasm-specific code, we need to respect the existing code formatting which isn't always clang-formatted, or locally reformat it as needed when making changes, so it's not a bad idea for us to get used to working that way anyway. And we accept patches that aren't clang-formatted.

I guess I don't have a strong opinion here. The code is already pretty close to clang-format's interpretation of LLVM style, so looking at the actual diffs you're right that it doesn't seem to be a big change. Changes like those in WebAssemblyLowerBrUnless.cpp are a little unfortunate, but tolerable.

The LLVM coding standards say "we explicitly do not want patches that do large-scale reformatting of existing code". They suggest that reformatting should be done to address specific readability issues or to prepare an area for subsequent changes.

In D50277#1210237, @aheejin wrote:

We discussed how to implement fences here, and I think the general consensus was even though it may not be strictly necessary to emit atomicrmw for some cases, it can be user friendly and also might help support legacy builtins like __sync_synchronize(). And IMHO it is a single instruction anyway and fences are generally expected to be expensive, so no significant harm will be done. Does anyone have more concerns on this? If not, can we land this?

This looks ok to land now.

Is this related to the issue reported in the thread here?

First, I apologize, I'm not sure how I lost track of this patch and didn't see the pings.

https://reviews.llvm.org/D40526 has now landed. Is there anything else we should do before leaving "experimental" status?

In D49517#1172783, @sbc100 wrote:

Interesting, so you are proposing delaying the error until link time but still producing a valid wasm binary?

In D49194#1172051, @jfb wrote:

I have a talk for you! https://youtu.be/IB57wIf9W1k?t=52m20s
I talk about volatile in a few places, but 52m20s is the main one. Basically I'm advocating for something similar to the weird MSVC x86 volatile behavior because some users write code like a loop which updates a volatile variable which another thread reads, and expect it to stay in the loop (not hoisted out). Lowering to regular load / store breaks the guarantee that volatiles effects touch the variable's bytes once per source event. Such user code should really use atomic, but we don't need to be pedants about it.

In D49194#1170428, @jfb wrote:

In D49194#1170425, @sunfish wrote:

For the record, I'm opposed to having the WebAssembly backend translate asm volatile(""::: "memory") differently from other targets in LLVM or GCC, which to my knowledge always translate it to a no-op, even on platforms with hardware fence instructions.

I think it should lower and not error out, and thought should be put into what it should lower to. No-op seems fine to me, but I'd like to hear a rationale.

For the record, I'm opposed to having the WebAssembly backend translate asm volatile(""::: "memory") differently from other targets in LLVM or GCC, which to my knowledge always translate it to a no-op, even on platforms with hardware fence instructions.

Ah, that's useful context, thanks. That'd be good to mention in the commit message and/or comments somewhere. Also, please add a testcase which approximates that use case.

Not all types are valid to cast to all other types. But beyond that, normal native platforms don't insert non-trivial casts in such situations, so even in many cases where it's valid to cast, it doesn't seem like programmers could reasonably expect a non-trivial cast to happen. What kinds of use cases are motivating this?

I'm not deeply familiar with MC's Asm parsing support, but this looks like a good direction to go.

I've now refreshed the emscripten patches and am hoping they can land soon.

Indeed; after discussing this with a few other folks, I think this is getting close.

My biggest concern here is that this pass shouldn't affect prototype-possessing functions, and that appears to be satisfied :-).

I haven't thought through all the possibilities related to !FD->doesThisDeclarationHaveABody(), but overall this looks good.

In D46805#1115681, @rsmith wrote:

In D46805#1113358, @aaron.ballman wrote:

@rsmith -- do the object file formats listed look correct to you?

They look at least plausible. We should be able to test whether LLVM can actually emit aliases on each of these targets easily enough...

However, I get this error for any WAsm compilation I try:

fatal error: error in backend: section size does not fit in a uint32_t

... so I have no idea if aliases are/will be supported there. Perhaps @sunfish can tell us :)

This looks right to me, but just to make sure I understand, has this been broken since r228400?

LGTM.

Looks good to me. Thanks!

This user-defined custom sections patch should be ready to go now.

We want to coordinate landing this with corresponding changes in Emscripten. Since this is an ABI change, Emscripten needs to figure out how to manage it. See here for ongoing discussion.

Indeed; I don't anticipate LLVM will be using mem.grow or mem.size itself. But, clang has __builtin_wasm_mem_grow and __builtin_wasm_mem_size, to allow standard libraries to use them. It's desirable that the names of these builtins, and the corresponding LLVM IR intrinsics, match the names in the official WebAssembly documentation.

We should probably also wait for the WebAssembly committee to settle on final instruction names.

I'm not very familiar with the Emscripten parts of this, but some grepping in the emscripten tree found these lines:

src/preamble.js:    env['memory'] = Module['wasmMemory'];
src/preamble.js:    env['memory'] = providedBuffer;
src/preamble.js:    assert(env['memory'] instanceof ArrayBuffer);

In D43212#1005708, @sunfish wrote:

The new tests look good, but it'd be good to keep testing "wasm32-unknown-unknown-wasm" and "wasm64-unknown-unknown-wasm" here too.

Please restore the "-wasm" forms of these tests too.

The new tests look good, but it'd be good to keep testing "wasm32-unknown-unknown-wasm" and "wasm64-unknown-unknown-wasm" here too.

I think it's getting closer. IMO we should at least land https://reviews.llvm.org/D43147 and https://reviews.llvm.org/D40526 first, and address any follow-up issues.

I think it's because writeBytes is defined to be passed data from an MCFragment's getContents(), which is a SmallVectorImpl<char>. That's the case here too, though the code that does the getContents() is a little separated from the writeBytes. Following @ncw's suggestion, I created a struct type, which not only avoids repeating the type name, but also makes the use of it cleaner than std::pair.