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.

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?

cc @kripken what do you think about the size-performance trade off here?

Looks good overall! Most of these comments are just me checking my understanding.

Nice! this looks like a good improvement for WebAssembly at least.

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

Thanks!

@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

• Reuse vt as suggested
• Add IntVecs as suggested

• Rebase on top of D93722

In D93660#2467233, @aheejin wrote:

Wow, I didn't know we can do OOP in TableGen! (Are these new functionalities?)

It would be good to add a description of the bug to the commit description.

Nice!

Looks good! Can you add a test that uses __wasm_start?

"not completely elided" => "completely elided" in the description.

Aha, that seems important!

"were being honored" => "were not being honored" in the patch description.

Looks good so far!

LGTM besides those last couple questions!

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.

Hmm, I guess we don't. Would you mind adding one?

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?

@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.

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++.

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?

LGTM if we can verify that neither Rust nor WASI use this functionality.

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!

Excellent, thanks! I'll land this.

In D90608#2369392, @sbc100 wrote:

In D90608#2369338, @tlively wrote:

In D90608#2369305, @sbc100 wrote:

lgtm. In a project that primarily deals with C/C++ code I find "heap time" a little confusing because it makes me think of the malloc heap used by C/C++. Why not "managed" or something like that to avoid ambiguity? I guess that is being discussed in the spec and we should probably just follow their lead.

I do think that matching the upstream spec "heap type" terminology is important, even if it's a little confusing in a C++ context.

Do you think that upstream term "heap type" is reasonable, given the pre-existing alternate meaning of heap that already exists in the unmanaged world?

In D90608#2369305, @sbc100 wrote:

lgtm. In a project that primarily deals with C/C++ code I find "heap time" a little confusing because it makes me think of the malloc heap used by C/C++. Why not "managed" or something like that to avoid ambiguity? I guess that is being discussed in the spec and we should probably just follow their lead.

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.

Thanks for all the reviews! That's the last of them for now :)