Thanks for your patience through all the review! Let's get this landed :)

In D101140#2789199, @wingo wrote:

Quick r? to @tlively -- OK with having the new util function in WebAssemblyFrameLowering? It didn't work to have it in the utils library because the function needs the vtable of WebAssemblyFunctionInfo.

LGTM with that one last comment on the test.

It would be good to add dedicated tests for table.get and table.set as well.

@sbc100 Is there anything better to do here besides falling back and emitting 0 as the offset?

Thanks for your review, @arsenm! LGTM from me as well.

In D102018#2743818, @aheejin wrote:

So what prevented us from using functions when we were using _Static_assert and why is it now possible to use functions?

This LGTM! MANAGED sounds like a good address space name to me. @sbc100, do you have any final comments?

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

In D101884#2739898, @aheejin wrote:

I see, thanks. Then other load instructions are OK not to be fixed?

• v128.load
• v128.loadN_splat
• v128.loadNxM_s/u

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.

• Remove (incorrect) const pointer changes. Will fix them and put them in a separate revision.

In D101684#2737842, @penzn wrote:

I think there is another dimension to this aside from project composition - intrinsics have a tendency to "interact" with their surroundings, and it better to capture the IR rather than the end result. Even if we can verify that simple calls produce instructions we expect, this might not hold true is the arguments change, or the call is in a different context. IR definitely gives more opportunities to test things through.

The Wasm changes LGTM! This is a nice simplification.

In D101850#2737379, @aheejin wrote:

Sorry I'm not sure if I understand. The "expected alignment" for all those memory instructions is 1, regardless of the size of the element of the vectors? Can you elaborate on why the previous code is incorrect?

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.

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.

In D101684#2732395, @dblaikie wrote:

In D101684#2732366, @tlively wrote:

In order to get the benefit of this end-to-end test from split tests like that, the LLVM test would have to be automatically generated from the clang test.

Why is that? We don't do that for other test surface area between clang and LLVM.

In D101684#2732310, @dblaikie wrote:

Assembly tests are generally discouraged in clang, but in this case we actually care about the specific instruction being generated from the intrinsics.

I don't think this is a sound reason to add an end-to-end test in clang. The same is true of all clang tests, right? We ultimately care that accessing a parameter lowers to a certain register (because we're trying to implement a certain ABI) but we don't test that in clang - we test that we lower to certain IR which is guaranteed to lower to a certain register use - and then in LLVM we test that that IR does lower to that register.

I think the same holds true here - and a clang test should verify the IR and an LLVM test should verify the assembly.

• 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?

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!

In D95425#2711969, @pmatos wrote:

Apparently getTypeForEVT in LowerArguments is only necessary to call the getABIAlignmentForCallingConv. I was thinking that since reference types really are pointers to different address spaces we could return their TypeForEVT as being int32. That will then return as the alignment for reference types the default alignment for int32. This should help with avoiding us touching middle end code. I will experiment in this direction.

In D101112#2710521, @aheejin wrote:

General question about SIMD intrinsics: So we make dedicated wasm intrinsics only for the cases there are not general intrinsics people can use instead?

• "stand" => "standard"
• Update builtins for v128.any_true

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.

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.