- User Since
- Jan 7 2013, 9:35 AM (280 w, 5 d)
Thu, May 17
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.
Wed, May 16
Tue, May 15
Also if you use the LINK_LLVM_DYLIB CMake mode (as our bots do) you should test the regular non-dylib static link build.
Thu, May 10
Mon, May 7
haven't looked at most of the code, but thought about the register operand issue.
Apr 6 2018
Apr 5 2018
Apr 4 2018
LGTM for the encoding... I hope the -1 offset wasn't being generated by the compiler though?
Apr 2 2018
Mar 30 2018
Mar 29 2018
Mar 28 2018
Otherwise it looks good to me, although @majnemer would know more about the subtleties of what IR actually gets generated.
Mar 27 2018
Mar 21 2018
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.
Mar 20 2018
- add braces in else
Mar 19 2018
Mar 15 2018
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?
Mar 14 2018
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).
Mar 13 2018
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.
Mar 12 2018
looking pretty good overall, here are a couple of high-level comments.
Mar 9 2018
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?)
Mar 8 2018
Hm, another option could be undef here. I don't know if that would be any easier though.
Mar 7 2018
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.
Mar 2 2018
Mar 1 2018
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.
Feb 16 2018
Yeah come to think of it, I thought we just failed isel for blockaddress?
Feb 12 2018
Feb 9 2018
- 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.
Feb 8 2018
I like this idea, did you abandon this CL in favor of something else?
Feb 5 2018
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 (!)
Feb 2 2018
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.
Jan 31 2018
Jan 30 2018
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.
Jan 23 2018
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.
Jan 22 2018
These are just the compile failures :) There's also https://github.com/WebAssembly/waterfall/pull/316/files#diff-a6006086cef2fc4f685647d1fe006766
Jan 19 2018
- use StringRef::withNullAsEmpty() and clang-format
Jan 18 2018
- fix build