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.

LGTM! (with single comment)

Nice! code looks good to me, but we'll need community signoff on an RFC as @rjmccall mentioned and it would be nice to have a clang maintainer sign off as well.

No concerns about the WebAssembly changes. Will the default installation for folks who don't touch LLVM_DISTRIBUTION_COMPONENTS change?

Great, thanks @jhenderson!

LGTM! This seems fairly reasonable as a solution, all things considered. Have you been able to check whether this has any broad impacts on the quality of our codegen? My guess is that it doesn't since it seems that any code that would have had the vector elements simplified previously would have ended up in that infinite loop, but we went a long time without ever seeing that bug in the wild.

Thanks, @jhenderson! You were right on all counts and I have the test working now. This should be ready for proper review.

• Update version check and available_features in lit.cfg.py

Right now (after trying to upgrade my gdb) I get this error

cc @Orlando and @jhenderson, who have worked on cross-project-tests. I've got ninja check-intrinsic-headers kind of working, but the test doesn't run because cross-project-tests/lit.cfg.py checks for a good gdb version (which I don't have) even though I don't want to run the debuginfo tests. Would you be able to help move that configuration code into cross-project-tests/debuginfo-tests?

@dyung, this should be fixed by https://reviews.llvm.org/rGc01ec3083026f7e24e6c06f48a05d413e2c697d4

This broke the build, working on a fix now.

Should we try to get some patch backported to the 13.x.x releases to get them out of the intermediate state that was causing this bug? I guess it's not really important after this fix. Or maybe this fix should be backported?

Who was previously responsible for applying the data relocs? If it was Emscripten runtime code, does that mean this change will have to land alongside the corresponding Emscripten change? Is there anyone who would be broken by this ABI break who we should notify?

An example situation where there might be more than one value is in a multivalue call, but Idx should only refer to whichever value is actually used as the offset and I agree that it shouldn't matter whether there are other values in the same node or not.

Code LGTM, but I don't quite understand what the bug was, so I'm not sure about the test coverage.

Thanks, @jingbao! I've landed this in https://github.com/llvm/llvm-project/commit/2a4a229d6dcceecbb8bab094b6880e2445a6e465.

@jingbao I was working on landing this, but the CodeGen/WebAssembly/fpclamptosat_vec.ll test had to be updated. Unfortunately Phabricator won't let me attach the changes to this revision, but it looks like this change introduces some new unnecessary masking that was not there before in a couple of the test functions. (It also makes a bunch of good changes!). Would you be able to take a look at that? I'd also be ok with landing this as-is since the improvements looks more significant and more common than the regression.

LGTM with the extra suggested test, if possible.

Thanks for doing this, @jingbao. Sorry I let it sit for so long. Do you have commit rights or would you like me to land this for you?

LGTM modulo comments. It's nice that we get to delete so much code this way.

WebAssembly test changes lgtm 👍

Also, it would be nice to have a test for this, but if it would be too much trouble, then I guess it's fine.

"globala" => "globals" in the descriptions (although globala sounds like a great name for something).

LGTM, thanks!

Oh yeah, I guess we could only do this if bulk memory is enabled.

Code LGTM, thanks!

Thanks!

LGTM!

Nice! Just a couple nits but I think this is good to go.

Thanks! I'll go ahead and land this.

Thanks, the WebAssembly portion LGTM. The TargetLowering.cpp change LGTM as well, modulo that one comment.