- User Since
- Jul 31 2018, 10:54 AM (149 w, 4 d)
Wed, Jun 9
Thanks for your patience through all the review! Let's get this landed :)
Mon, May 31
Thu, May 27
LGTM with that one last comment on the test.
Mon, May 24
It would be good to add dedicated tests for table.get and table.set as well.
Fri, May 21
@sbc100 Is there anything better to do here besides falling back and emitting 0 as the offset?
Thu, May 20
Thanks for your review, @arsenm! LGTM from me as well.
May 12 2021
May 7 2021
May 6 2021
This LGTM! MANAGED sounds like a good address space name to me. @sbc100, do you have any final comments?
May 5 2021
@sbc100, you know a lot more about this symbol stuff than I do. Could you take a look at this?
LGTM with Sam's comments resolved!
@dblaikie, what's the best practice for making tests robust to this difference?
It only matters for pointer parameters. For example, const int * is meaningfully different from int *, but const int is not usefully different from int.
Thanks, @hans! That's very surprising; I'll take a deeper look at what was going wrong.
May 4 2021
- Remove (incorrect) const pointer changes. Will fix them and put them in a separate revision.
The Wasm changes LGTM! This is a nice simplification.
At just under 4 seconds on my machine, it's certainly not one of the quickest clang lit tests, but neither is it one of the slowest. It looks like there are many that take tens of seconds.
May 3 2021
I chatted with @dblaikie offline about this just now, and we both think it makes sense to turn this particular test into a C->IR test, then later potentially add a C->Wasm end-to-end test to the cross-project-tests directory created in this WIP stack of diffs: https://reviews.llvm.org/D95339. I'll also bump the RFC thread about cross-project-tests with a pointer to this conversation to solicit more feedback about whether this kind of end-to-end intrinsic test should be in scope for cross-project-tests.
This is hopefully less controversial because the test already exists and this is just re-enabling it.
Oh right, this only changes the register-based printing, which is used exclusively in llc tests. The stacky assembly that would be used in production is unaffected, so it's not surprising that none of the existing assembly tests are affected.
May 2 2021
Apr 30 2021
- squash to include all changes
@sbc100 Will fixing the assembly format to have the correct operand order like this break Emscripten at all? I think the incorrect order also made it into the LLVM 12 release, but I think we have few enough users that it's probably worth fixing now.
Nice! I think that splitting this out of the change that adds reference types is a good idea. Regarding the FIXME in the test, is it the case that the globals are also not emitted in the binary format, or is it just the assembly output that is broken?
Apr 28 2021
Nice! This turns out to be relatively simple. It would be good to add more tests with reference typed locals mixed in with normal locals, etc. Also, what happens when one of the alloca values is used by something other than a load or a store? Is it just an ISel failure at that point?
This is looking really good!
Apr 27 2021
Apr 23 2021
- "stand" => "standard"
- Update builtins for v128.any_true
Apr 22 2021
Thanks for the context links! This LGTM 👍
The code looks fine, but explicitly requesting a review from @arsenm because I don't know the history of this restriction or how this affects other targets.
Apr 20 2021
Apr 19 2021
I'm still not comfortable approving this patch with changes to llvm-c/, Bitcode/, or IR/. If those changes can't be backed out, this will require an RFC on llvm-dev, but I would be surprised if that were necessary.