- User Since
- Jul 31 2018, 10:54 AM (129 w, 11 h)
Fri, Jan 15
Mon, Jan 11
Looks good overall! Most of these comments are just me checking my understanding.
Nice! this looks like a good improvement for WebAssembly at least.
Fri, Jan 8
Thu, Jan 7
Tue, Jan 5
In the description I think "but LLVM does not have a way of that kind of behavior" is missing the word "modeling" => "but LLVM does not have a way of modeling that kind of behavior"
Looks good! A few of the replacements in the code don't match the replacements in the comments, but I assume the comments were previously just stale.
- Fix section headers in test file
Mon, Jan 4
Mon, Dec 28
- Fix alignment
Wed, Dec 23
- Reuse vt as suggested
- Add IntVecs as suggested
Tue, Dec 22
- Rebase on top of D93722
Mon, Dec 21
It would be good to add a description of the bug to the commit description.
Dec 10 2020
Looks good! Can you add a test that uses __wasm_start?
"not completely elided" => "completely elided" in the description.
Dec 9 2020
Aha, that seems important!
Dec 7 2020
"were being honored" => "were not being honored" in the patch description.
Dec 3 2020
Looks good so far!
LGTM besides those last couple questions!
Dec 1 2020
Oh right, I forgot about those crazy unreadable tests. Sam is totally right. For now adding new RUN and CHECK lines to those tests should be sufficient.
Nov 30 2020
Hmm, I guess we don't. Would you mind adding one?
Nov 19 2020
Nice! This will be helpful for reading disassembled modules. In a future where we have a clang extension allowing users to define their own WebAssembly globals from C/C++, would it be easy to extend this to support names for those arbitrary globals as well?
Nov 17 2020
@arsenm, Are you suggesting that we just relax the verification rules to allow calling function pointers to arbitrary address spaces without needing any changes to the data layout string? That seems fine to me, but the data layout string solution does allow targets to opt in to more precise validation.
Nov 13 2020
I think this is a good direction overall, and I'm glad the code doesn't become any messier with this change. I do think it would be good to also email llvm-dev about this change to get general feedback and make sure it doesn't require a full RFC.
Wow, that bug is wild!
Yes, this looks better than what we had before. I think it would be generally better to use TableGen patterns rather than generating the MachineNode directly in C++.
Nov 12 2020
Why are there separate versions of the new relocation for both i32 and sleb128? Do we use both of them?
Thanks, Alex! I suspected that might be the case. Follow-up question: What kind of binary compatibility guarantees does Rust have when mixing compiler versions? Would it be a problem for example if we rewrote the TLS mechanism from scratch so that it worked differently in LLVM 12 than it does in LLVM 11?
Nov 11 2020
LGTM if we can verify that neither Rust nor WASI use this functionality.
Nov 10 2020
Excellent, thanks. I hope I'm not forgetting any good reason we added these. Emscripten doesn't use them in its pthreads runtime, right?
Nov 3 2020
Excellent, thanks! I'll land this.
Nov 2 2020
Nice! This looks good besides that one question. Also, for the patch description, it seems slightly misleading to say that "the register form of ref.null is still unimplemented"; IIUC, the register form is implemented since the I tablegen multiclass creates both forms, but it's simply not used for anything right now.
Oct 30 2020
Thanks for all the reviews! That's the last of them for now :)