I think I actually prefer the approach of linking this code in from a library rather than generating N copies of it. It's more consistent with how system code is usually included into programs (i.e. linking it in rather than generating it magically from the compiler). It's also easier to read and understand for non-compiler developers.

@jfb just as a sanity check, it should always be valid to just "upgrade" LLVM's orderings and select them all as the sequentially consistent wasm ops, right?

SGTM

There should also be a few tests that orderings other than seq_cst get selected.

This change is really quite nice and minimal other than the renaming.

Otherwise LGTM

A nit about the commit message: it's not just LTO passes that make assumptions about known library functions, lots of passes do. Also I might be just a little bit more explicit than "when statically linking libc" and make it "when including libc in LTO"
Otherwise LGTM

OK, the pass looks good; don't know if @majnemer has any more feedback on this or D44134?

Looks pretty reasonable otherwise. I like the MIR unittest 👌

LGTM with the tweak to the comment.

Since it fixes the build and is a trivial change, I think you should just go ahead and commit this.

still LGTM

LGTM; I think this model definitely makes more sense in CFG-based IR.

Otherwise LGTM

This does seem a little odd, but I think it makes sense given the special nature of functions in the wasm object format.

LGTM, we can remove any code relating to binary output with the ELF triple now. Hopefully not much longer before we can remove the ELF triple entirely.

No worries; the WebAssembly target is still an "experimental" target and doesn't build by default (even you build "all" targets) unless you also add -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD. So this happens from time to time.

Yes, still LGTM

Otherwise LGTM

LGTM assuming we are convinced for now that finding the rethrow block from the CatchSwitch will work. Does this need to wait until after https://reviews.llvm.org/D43746 or other CLs from that chain, or does it matter?

The Wasm code looks good with the one comment so LGTM if @majnemer is OK with the modification to WinEHPrepare.

Thanks for your patience on this, I really do think this is a big readability improvement.

LGTM, thanks

Also if you use the LINK_LLVM_DYLIB CMake mode (as our bots do) you should test the regular non-dylib static link build.

In D45559#1094868, @aheejin wrote:

Let me clarify. So there are the two options:

1. Use 'funclet' to only mean outlined funclet in AsmPrinter I guess @dschuff endorses this, right? I'm not sure what @majnemer meant. Actually this is what I first tried to do in this patch, but this seems to be a harder way now, because as I said, the term funclet is used to mean a lot of things and being used everywhere including both llvm and clang in all sorts of method names in comments, not only this personality functions.
2. Leave the uses of 'funclet' as they are, delete usesWindowsEHInstructions function and change the uses of them back to isFuncletEHPersonality, and create something like usesFuncletLSDA to be used in the cases that only pertain to real outlined funclets. This seems to be less work, because we don't need to change the uses of 'funclet' everywhere...

In D45559#1091152, @aheejin wrote:

But currently the term 'use funclets', both in function names and comments / error messages, is being used to mean both 'use windows EH IR' and 'use outlined funclets' in the code.
Currently 'use funclet' can mean either thing in LLVM. But in wasm we only want the IR level behavior but not the outlining behavior. An example of the latter, which deals with the real function outlining, is this code preparing information for LSDA generation for funclets, which we are not gonna use in wasm.

haven't looked at most of the code, but thought about the register operand issue.

LGTM for the encoding... I hope the -1 offset wasn't being generated by the compiler though?

Otherwise it looks good to me, although @majnemer would know more about the subtleties of what IR actually gets generated.

Sorry about that. Although I'm confused though why setting 'ShouldEmitMatchRegisterName' (which I added right before commit when I saw this warning myself) (https://github.com/llvm-mirror/llvm/blob/master/lib/Target/WebAssembly/WebAssembly.td#L84) didn't work.

• add braces in else

LGTM.

I think i'd be in favor of letting this patch go in as it is for now, since it uses our existing syntax, and then have a separate discussion of how to change it on tool-conventions.

Do you want me to commit this?

Otherwise LGTM

I think the symbol table (and any names that actually have meaning to the compiler and/or linker) should be mangled names, and only the name section should have demangled names. I also think it makes sense for libObject to only use the symbol table and ignore the name section. In other words, the name section should be thought of as metadata or debug info and in that sense shouldn't be "trusted" to always be correct (although of course we should preserve it wherever we can).

LGTM, and confirmed that it makes libcxx configure successfully.