• Add encoding tests

@sunfish I believe you originally added the GlobalAddress patterns so I wanted to check in with you before removing them. Do you know whether they can ever be used? If so I will add tests for them instead, but I'm not sure I've ever seen a GlobalAddress offset.

• Add test for custom section names and change sorting to handle them

In D67639#1671921, @aheejin wrote:

What are the default values for those if we unset them?

In D67425#1668831, @aheejin wrote:

LGTM.

• Maybe we can lower these to these new instructions later? (I'm not very sure what the difference between versions with and without VECTOR, because the versions without VECTOR seem to be able to take vector arguments as well.)
• Will this simplify the complicated lowering of SIGN_EXTEND_INREG?

LGTM, thanks!

• Make narrows binary ops

I missed the fact that the narrows are supposed to be binary operations. Oops.

Thanks for the clarification. It makes sense to me that -mexception-handling only enables the architectural feature and a separate flag enables the behavior change. This is indeed consistent with how -pthread works.

Is there a plan to turn this on by default at some point, or will users always have to opt-in to using exceptions? Also, would it make sense to automatically turn this feature on if the user instead supplies -mexception-handling to turn on the target feature?

In D65783#1656699, @sbc100 wrote:

Can you verify that you can pass all the "wasm2" tests in emscripten with this change?

• Address review comments

• Switch to more robust scheme with cmpxchg/wait/notify

Abandoning in favor of @sunfish's idea for introducing a cost mechanism for shuffle masks in DAGCombiner.

@sunfish That sounds like a useful mechanism to me. I'd be happy to work on that next week. Is the consensus that we should not merge this change and instead pursue that idea?

In D67020#1653403, @aheejin wrote:

Should we handle these too?

• There seems to be a generic intrinsic for fused multiply-add: https://github.com/llvm/llvm-project/blob/d21a3e41a4cfd52e3c5c9341f0b5ce8a173198bf/llvm/include/llvm/Target/GenericOpcodes.td#L612-L619
• There are FMA related nodes in ISDOpcode.h, such as
  • https://github.com/llvm/llvm-project/blob/1ea909270c6d9ad612a46c2a15c13acc4f6d623a/llvm/include/llvm/CodeGen/ISDOpcodes.h#L332-L337
  • https://github.com/llvm/llvm-project/blob/1ea909270c6d9ad612a46c2a15c13acc4f6d623a/llvm/include/llvm/CodeGen/ISDOpcodes.h#L294

Link to DAGCombiner.cpp code: https://github.com/llvm/llvm-project/blob/802aab5/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L19014

They need benchmarking data before they can be merged into the proposal, and they need to be supported in the toolchain for us to get good benchmarking data.

The context for this CL is https://github.com/emscripten-core/emscripten/issues/9340. The code that does the undesirable optimization is around llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18776. I think it is a reasonable assumption that what you put in is what you get out, especially if you're trying to trigger particular WebAssembly engine behavior that the backend should not really be reasoning about. Now perhaps users should not be reasoning about it either, but given that they are I think this patch is reasonable.

Generally LGTM, although the tests might be more readable if you pass -wasm-disable-explicit-locals and -wasm-keep-registers as well. It's also concerning that no one noticed this issue before. Do you know why other targets may not have run into it?

Thanks for you comments! I am also planning to update the memory initialization scheme to use wait and notify so that it works with racing modules instead of requiring the runtime to ensure that no module is called into before the main module is finished initializing. In a web context the runtime will have to ensure the main thread wins this race because it is not allowed to wait, but this is still strictly more general than the current scheme. Does that sound good to you?

In D65887#1619471, @RKSimon wrote:

@tlively Another thing we could consider is adding WebAssemblyISD opcodes for zero extending extract vector element - similar to what X86ISD does with PEXTRW/PEXTRB - this would avoid the need for the (and, extract_element(v, c), 0xFFFF) pattern that is causing all the problems.

It would be good to add a test here, too ;)

Sounds good; I'm happy to help out.

Oops it would be good to have a test, too ;)

I have mixed feelings about this. One one hand I understand that this file is huge and cross-cutting changes could cause a lot of manual updating work, but on the other hand each individual test was very readable and succinct. The auto-generated version would certainly be less work to maintain, but its enormous amount of irrelevant information and repetition is unfortunate. @aheejin, what do you think about this?

LGTM with a small test format change.

In D65463#1607742, @aheejin wrote:

I like this approach. I prefer the option 2 among the three options you described: 1 is redundantly doing what ISel is already doing, and 3 makes LLVM-backend generated code incorrect so we have to rely on the other tool to fix it, which seems not very desirable. As you said, having to add an extra argument to the interface of CanLowerReturn is a bit sad but I don't really mind that if others are OK with it.

@dschuff @tlively Any more opinions?

This looks great! The code is straightforward and I like the extra information in the text format. We will have to do a little more work to support return_call_indirect, but it looks like doing so should be fairly straightforward on top of this base.

• Update documentation

• Do not allow varargs tail calls

• Address comments, add a few more tests

• Record proper return types for indirect tail calls as well

Nice

• Simplify flags logic

LGTM modulo a test!