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.

I don't think import or export should be on by default (even if emscripten decides to do that) but the option should be there. It's still pretty early days for the VMs and just because they don't optimize better today doesn't mean they never will.

looking pretty good overall, here are a couple of high-level comments.

Sure, but once everything is in, then we shouldn't have the problem that we get undefs created. So we coudl just roll this into whatever CL adds the defs.

We don't actually create uses without reaching defs though, do we? Isn't it true that by the time we run PrepareForLiveIntervals, we've lowered the catch (and the catch will create a def of the phys reg that reaches the rethrow?)

Hm, another option could be undef here. I don't know if that would be any easier though.

Everything else seems pretty straightforward.

I forget what the spec says right now, but I think we will need a null value for except_ref in the spec anyway, because locals are implicitly initialized to their respective 0 values. So we'd need a corresponding 0 value for ref types. So I think it would be fine to have a nullref type in LLVM too.

Sorry this slipped through; this looks right to me.

Why do we need to omit the argument here? I'm not sure I understand what you mean by adding it after isel. Can/should a pass add an argument that doesn't match the format in the td?

This flag turns on CPU features (i.e. there's one for each new wasm feature proposal that has to be feature-detected), so I think this makes sense to be consistent with all the others. I could imagine enabling exceptions in the frontend but having some kind of emulation in the backend (like we do today for emscripten). More generally the semantics -fno-exceptions unfortunately doesn't exactly match the kind of behavior people typically want because it doesn't allow you to compile code that has try/catch/throw at all. In PNaCl and emscripten the default behavior is instead to compile that code but to lower it away and turn throw into abort. Also IIRC even with -fno-exceptions the code still has to allow exceptions to propagate which means you end up with invokes everywhere instead of calls, so you are still paying most of the costs of enabling EH. So that bit is a little bit complex and we'll probably want to think a bit more carefully about what options we want to have. But a machine flag for enabling the CPU feature makes sense to start.

I'm fine with doing this change since it does reflect the spec. Although IIRC the relevant types don't yet have any values over 128 and we've kept it that way so we can always upgrade to varint when we need to. So in principle we could keep the code using varint and then it won't have to change in the future.

Yeah come to think of it, I thought we just failed isel for blockaddress?

• My reading of the C standard is that using a prototype less declaration and defining that as a vararg functions is invalid in C. If you find bugs in this area we should fix them.

Interesting, I'll look closer at that. We haven't gotten any complaints about it so far.

• This patch simply removes all prototype less functions which is not fine.

Sorry, in case I wasn't clear, I agree that these tests are valid C and we should not change them.

• The llvm IR coming out of clang appears to mark all prototype less functions as varargs which from what I suspect makes it impossible to implement varargs different from prototype-less functions. But that is a problem in clang/IR, it doesn't change the fact that the tests are valid C.

Yeah, you can sort of cheat because in C, vararg functions must have at least one non-vararg parameter (at least AFAIK no C standard implemented by Clang allows that). So you can tell a real vararg function declare i32 @foo(i32, ...) from a non-prototype function declare i32 @foo(...). I suspect that only works for C though.

I'll just add that what we do in WebAssembly right now for unprototyped functions is to codegen them using the fixed-arg calling convention; at link-time if there are any mismatches between the definition and any calls, then the program has UB, and the resulting WebAssembly module will be invalid. I think this currently has the problem that if the definition does turn out to be vararg, then we have generated an invalid module even though the program is correct (I consider that a bug or shortcoming in our toolchain though). In any case, we do aim to support correct programs with prototypeless functions. If the test suite has non-vararg cases where there are mismatches between the callsites and/or the definition (I have seen that in some other test/benchmark suites), then even though that works on most X86 and ARM ABIs that I know of it's still UB and I'd be in favor of removing it from LLVM's test suite.

I like this idea, did you abandon this CL in favor of something else?

So, it turns out that this actually does change functionality on targets (such as WebAssembly) that use the expansion for unordered comparisons because it tries inverting the condition before falling all the way through to line 1680. This actually results in better code for wasm; see https://reviews.llvm.org/rL324305

@sunfish This is really just a change-detector test for the cond code expansion behavior that changed in https://reviews.llvm.org/D42788. Should we consider loosening it so that it just tests that we used the expansion? OTOH it's a bit concerning that that change didn't affect any other tests (!)

I do like the idea of having a linker-generated symbol instead of metadata for _end (or _wasm_end or whatever) and it's something that is unlikely to be broken in the future. So I think we should do that. But I do agree that specifying all these things would limit our ability to improve the layout in the future, especially wrt the order of the static/heap/stack bits. Since we have more constraints on our memory space than ELF (e.g. expanding only at the top instead of mmap), we might want to retain that flexibility, or even make things more dynamic or configurable, which would probably argue against matching the behavior of ELF here. We could consider having separate start/end markers for particular sections instead of having implicit ordering, if there's a use case for that.

In D42095#993612, @sbc100 wrote:

This change does not effect lld because the wasm "table" in
object file format is purely cosmetic and lld ignores its
contents.

In D37831#992822, @sbc100 wrote:

After a little discussion about this and the -gc-sections linker flag, the augments for the tools having sensible defaults seem to be winning.

I think LLD should probably default to no gc, and in general to minimal/most simple options, and the smarts should be in the driver. I think this matches other targets, and I think it also matches (or at least, should match) the way we have options the frontend/backend vs the driver; i.e. IIRC the driver has logic to pass -ffunction-sections and other codegen options to the frontend. So basically all the special logic goes in one place.

LGTM FTR, as long as we coordinate when this lands with emscripten.

I was going to say that most llc tests in LLVM don't use mtriple and that we should be consistent with that, but a quick grep suggests that something like 65% of the RUN lines with llc actually do contain mtriple. So I guess we could go either way. I think we still probably want to keep the triple in the bitcode (it is more convenient to run just llc, but only marginally so), in which case mtriple would be redundant. But I don't have strong opinions.

These are just the compile failures :) There's also https://github.com/WebAssembly/waterfall/pull/316/files#diff-a6006086cef2fc4f685647d1fe006766

• use StringRef::withNullAsEmpty() and clang-format

• fix build