User Details
- User Since
- Jul 31 2018, 10:54 AM (130 w, 11 h)
Today
Yes, I agree. We advertise a stable object format with the goal of making the linker forward-compatible. The value we get from that is that users with different toolchain versions (for both Emscripten and WASI) can share libraries without worrying about version skew in either direction as long as a least common denominator of features is used.
I'm happy to see that this patch improves the WebAssembly code, but I am not familiar enough with the relevant code to comment on the rest of the patch. No concerns from me.
Yesterday
Fri, Jan 22
This commit appears to be the source of ongoing object file ABI breakage that is causing older versions of lld that do not know about table index relocations to fail to link objects produced by versions of LLVM after this change. This can be fixed by only adding the __indirect_function_table import when the reference-types feature is enabled.
@dschuff, do you have an opinion about this?
Thu, Jan 21
cc @kripken what do you think about the size-performance trade off here?
Wed, Jan 20
Tue, Jan 19
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
Nice!
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
Thanks!
Mon, Dec 28
@fhahn A git bisect points to this commit as introducing https://bugs.llvm.org/show_bug.cgi?id=48616. Would you be able to take a look?
- Fix alignment
Dec 23 2020
- Reuse vt as suggested
- Add IntVecs as suggested
Dec 22 2020
- Rebase on top of D93722
Dec 21 2020
It would be good to add a description of the bug to the commit description.
Dec 10 2020
Nice!
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?
Nice, thanks!
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 :)