- User Since
- Jan 7 2013, 9:35 AM (297 w, 4 d)
I like the idea of a consistent naming scheme with double-underscores for all of those special symbols. I also don't see any reason emscripten shouldn't use those names too. If it's easier, we could just prepare both an emscripten and an LLVM change and land them more-or-less at the same time and avoid a 3-stage process.
Fri, Sep 14
Is this enough to add SIMD to one of our existing binary-format tests?
Are there tests that trigger these? It might make sense to add a case at least to the explicit locals and reg stackify. Probably fast-isel support should be a separate change (you can take the existing SIMD tests and run the same test, forcing fast-isel as some of the other tests do)
Wed, Sep 12
Thu, Sep 6
Yeah, rL341580 looks almost identical to this.
Passing it as a parameter does sound better.
- Pass the binary name as a parameter instead
Tue, Sep 4
LGTM to ensure we are using the stack instructions.
It's unfortunate about the type index requirement. That will make it quite a pain to hand-write assembly files. It would be nice if we could eventually figure out a way to symbolize those like the wat format.
I took another look at the diff, and it does seem like we are fairly close already. I agree that some of the changes to the switch statements aren't great, but on balance it's probably worthwhile to be able to just run clang-format and accept whatever it generates. I think we can go ahead and land this.
Fri, Aug 31
Thu, Aug 30
Wed, Aug 29
If we want to make concerted progress toward full clang-formattedness (I like the idea in general, but opinions sometimes differ on the value of that) we could do it one file at a time, to be landed before making some change to that file. That would be a sort of compromise; it would keep the changes small and manageable, but it would be a larger granularity and scope than what you'd strictly need if you were going to make a small change or just work on one component (which would mean faster progress).
Tue, Aug 28
Mon, Aug 27
Aug 22 2018
Otherwise LGTM, although I'm not an expert in DebugInfo either.
This should probably have some tests. It looks like there are some existing x32 tests in test/Driver/linux-header-search.cpp and test/Driver/cross-linux.c that might be relevant for updating.
Aug 21 2018
Aug 20 2018
This looks OK to me; I can't think of any good alternative to adding a new predicate. Fixing PEI directly would require having it check for particular instructions (wasm catchret), which we can't really do.
Aug 16 2018
Aug 15 2018
Oops, retroactive LGTM
Aug 14 2018
also please rebase against master.
Was the problem just that the PEI was inserting an epilog for every instruction that isReturn?
Aug 8 2018
Aug 7 2018
Keeping MUL is fine for now. IIRC there was some discussion in the last CG in-person meeting about removing or changing it (e.g. probably not super-useful especially for small types due to the likelihood of overflow). Probably in cases where the spec is still in flux it makes sense for LLVM to match what V8 implements now, so we can test things end-to-end. Then when we settle on things we can just change LLVM (or do LLVM first to see whether a proposal is feasible).
Aug 6 2018
Great that the atomicexpandpass just works for this.
LGTM With those changes.
Aug 2 2018
Still looks good 👍
Aug 1 2018
I would imagine that's already on Ben's plate to do sometime, but yes it should be changed to match JS.
I like this approach too, assuming it's sufficient to handle the CMake (and autoconf?) ugliness. I am a bit concerned that this pass will become a large and difficult-to-maintain pile of heuristics with different purposes, so I think we should be as clear as possible about what use cases we are trying to handle.
Other than the comment nit, the code looks fine too.
Jul 31 2018
Note that in JS, atomics.wake is now atomics.notify (https://github.com/tc39/ecma262/pull/1220). So we should use the new terminology for new and internal stuff. Since there's no legacy yet (except maybe in Binaryen's and WABT's text parser) hopefully LLVM can just use "notify" everywhere.
Jul 27 2018
Jul 12 2018
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?
Jul 9 2018
There should also be a few tests that orderings other than seq_cst get selected.
Jul 3 2018
This change is really quite nice and minimal other than the renaming.
Jul 2 2018
Jun 22 2018
Jun 21 2018
Jun 20 2018
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"
Jun 19 2018
Looks pretty reasonable otherwise. I like the MIR unittest 👌
Jun 18 2018
LGTM with the tweak to the comment.