Yeah, I agree that many tests are better with smaller, manually written expectations. For these particular tests, the generated expectations aren't that long and are easy to scan for the expected instructions. Using the full output also helps show that the arguments, epilogues, and other surrounding code looks good as well.

In D146569#4211332, @aheejin wrote:

Someone uploaded a CL fixing this a while ago (D142615), and not sure why they didn't land it. Anyway LGTM.

Got it, thanks.

Oh oops, I had written a response but then I never clicked "submit." Sorry about that.

Thanks! I didn't know about this functionality. Is there a generally accepted mechanism to mark intrinsics as experimental so that there is no expectation that they will be auto-upgraded like this?

Code LGTM, although it looks like there may be some missing cases reported in https://github.com/llvm/llvm-project/issues/59095. Testing the end-to-end integration here might be a good thing to do in cross-project-tests.

LGTM besides using the test update script.

For the description, here's some possible clearer wording:

LGTM % language tweaks!

Great, thank you!

In D139010#4158540, @aaron.ballman wrote:

Roping in @jfb because I know he's been heavily involve in WebAssembly in the past and he may have ideas/opinions.

High-level question: are externref, funcref, and table types all part of the WebAssembly standard, or are these extensions? (I wasn't aware that WebAssembly was a W3C standard or I'd have been asking this question much earlier -- sorry for that! I'm asking in regards to #4 in https://clang.llvm.org/get_involved.html#criteria)

Thanks for the ping! Sorry this fell through the cracks.

The clang driver will automatically call wasm-opt if it finds it in your PATH, so it's worth checking whether wasm-opt is being run or not.

Yes, pmin is rhs < lhs ? rhs : lhs and pmax is lhs < rhs ? rhs : lhs, so if either input is NaN, the result for either operation should be lhs. IIUC, before this patch NaN inputs would result in rhs instead.

Great, thank you!

I don't think this is correct with respect to NaN vector elements. SIMDe is getting test failures starting with this commit, reported here: https://github.com/emscripten-core/emscripten/issues/18759

LGTM, thanks!

Impressive performance wins! How is code size affected?

FWIW I had to patch the musl used by Emscripten to work around this: https://github.com/emscripten-core/emscripten/pull/18510

Could you add a test showing the regression in a preliminary patch so that we can see the improvement in this patch?

LGTM % comment. Thanks for taking this!

Nice!

LGTM

Nice, it's good to use the builtin nodes where possible. I think we had intentionally selected splats of constants into constant vectors for performance reasons. Would you be able to preserve that behavior and make this a non-functional change?

LGTM. It definitely makes the tablegen simpler, and I think the C++ isn't that complicated.

• Fix type

LGTM, thanks!

In D137824#3924406, @samparker wrote:

I don't know if you've seen it, but we have a multivalue-stackify.py script that tries to exhaustively enumerate multivalue sequences up to a certain size then filter out the uninteresting ones.

Cool! My python isn't great tbh... but looking through the generated test, I would have thought that f141 may have triggered this bug:

%t0 = call {i32, i32} @op_0_to_2()
%t1 = extractvalue {i32, i32} %t0, 1
call void @op_1_to_0(i32 %t1)
%t2 = extractvalue {i32, i32} %t0, 0
call void @op_1_to_0(i32 %t2)
ret void

%0:i32, %1:i32 = CALL @op_0_to_2, implicit-def dead $arguments, implicit $sp32, implicit $sp64, implicit-def dead $arguments, implicit $sp32, implicit $sp64
CALL @op_1_to_0, %1, implicit-def dead $arguments, implicit $sp32, implicit $sp64, implicit-def dead $arguments, implicit $sp32, implicit $sp64
CALL @op_1_to_0, %0, implicit-def dead $arguments, implicit $sp32, implicit $sp64, implicit-def dead $arguments, implicit $sp32, implicit $sp64
RETURN implicit-def dead $arguments

Is it the fact that the users are calls and this prevents a move from being attempted..?

I don't know if you've seen it, but we have a multivalue-stackify.py script that tries to exhaustively enumerate multivalue sequences up to a certain size then filter out the uninteresting ones. The output is checked in as the multivalue-stackify.ll test. It may be worth playing around with that script so that it generates programs that exhibit this problem.

LGTM % comment. Thanks!

In D135831#3856024, @sbc100 wrote:

In D135831#3856019, @tlively wrote:

It seems kind of strange to have features be free form strings rather than a set of flags, but maybe that's appropriate at this stage. It's certainly more flexible.

But that direction was already taken with the existing --features= flag right? I'm not adding that concept here.

It seems kind of strange to have features be free form strings rather than a set of flags, but maybe that's appropriate at this stage. It's certainly more flexible.

LGTM!

Thanks! Do you need me to land this?

Yeah the unrelated test failures can be ignored. Good point about our shuffle combine workaround, @penzn. Would you have time and resources to investigate whether we can remove the workaround now?

Sounds good. @fanchenkong1, it would be great if you could expand the comment as @penzn suggested. After that, do you have commit access or would you like me to merge this for you? If you would like me to commit it, what author email should I use for you?

The code looks good, but I don't know much about native SIMD. Is this strictly better for engine-side codegen on all platforms? LGTM if so.

It would be good to add tests for more error conditions like in the externref patch and also additional errors like trying to apply __funcref to types that aren't function pointers.

Very nice! This LGTM with all these small comments addressed. Sorry for the long delay in reviewing.

For reference, the existing error from the backend is something like this:

Oops, sorry about that. Will take a look.

Nice! LGTM with those comments.

I think it would make sense to add a new method to TTI for this, since hardcoding an exception for Wasm is not going to be future-proof at all. The tail-call target feature, while not standard yet, is very close to being standardized and shipped in browsers. It's already available behind a flag in Chrome and Node. Also, if possible, it would be good to perform a different transformation that can still do symmetric transfer without tail calls if the prevailing view is that symmetric transfer is guaranteed by the spec (but simply omitting musttail for now seems like a reasonable stopgap measure).

In D128282#3601100, @asb wrote:

In D128282#3600355, @tlively wrote:

I think the lines still differ in that one tests wasm32 and the other tests wasm64 (the triples are different).

Yeah, but the convention used elsewhere in this test file is to use WEBASSEMBLY: (which is a common check prefix for both wasm32 and wasm64 run lines) in the cases where the output is the same.

I think the lines still differ in that one tests wasm32 and the other tests wasm64 (the triples are different).

Thanks for taking a look at this!

• dot_add takes three operands

Thanks, this makes more sense now. LGTM with fixes and extra information in that comment.