• clang-tidy fix

Yeah, that sounds better, if that does not involve too much effort.

I took a little more look at this and all of this cost model is very confusing. :( First I don't understand why the lists of intrinsics listed in switch-cases in getIntrinsicInstrCost and getTypeBasedIntrinsicInstrCost are different. fshr is listed only in getIntrinsicInstrCost but not in getTypeBasedIntrinsicInstrCost. It uses some more info than just types (like if some value is power of 2 or something), but even if we don't assume anything, it returns a far greater number (in this case 5) because it computes and accumulates cost for each of its argument while getTypeBasedIntrinsicInstrCost does not have a case handling for fshr so it just assumes it is cheap and returns 1.

Not sure if it's worth it if there's no precedents. If people use our toolchain (clang or emscripten or rust or whatever) that wouldn't happen in the first place. Maybe it'd be sufficient to print some warning messages in CoalesceFeaturesAndStripAtomics if features don't match..?

Where are you planning to run that pass? Inlining runs within opt, before we can have any chances to run our own passes. You mean clang by 'frontend'? Can we run our own pass in clang?

I think D79941 is supposed to work; what it did is move those logics into constructors of IntrinsicCostAttributes. For example, this constructor saves FMF and argument types, which are supposed to be used later in the same way. I think it'd be better to figure that out and fix it than restore the old code? Maybe the original author @samparker has some ideas on why :)

Nice! Just wondering, then are we able to fold most of our SIMD intrinsics here too?

+1 on only inlining into functions which have a superset of features; it looks safer that way.

It looks it was already addressed by https://github.com/llvm/llvm-project/commit/1603470e59a99a39ebdc4bf62a3a16c8c4ebea36.

This looks fine to me, but I haven't worked on this part of code myself, so probably others who have can provide better review.

It looks like very smart transformation!

Aha I see. Thanks.

They seem to have been written that way because negative offsets aren't folded? What does it mean that they are duplicates of other patterns?

LGTM.
Nit: NFC usually means code change that does not affect output, so adding tests or changing tests is not NFC by that definition :)

LGTM, but I posted a question on whether we need performBUILD_VECTORCombine in D83606.

Wouldn't we want to add ISD::ABS to scalarizeSplatValue method in DAGCombiner.cpp too? WebAssembly wouldn't benefit from it, but it can benefit other targets that have a scalar abs instruction, and it is also consistent.

I think this is a nice idea! But I'd like people working on other targets to check this too.

• Align FileCheck lines

@MaskRay Thanks!

In D81909#2099263, @MaskRay wrote:

Hi, you can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (I have updated my script to use --date=now (setting author date to committer date))

https://reviews.llvm.org/D80978 contains a git pre-push hook to automate this.

Sorry for the delayed reply!

Yeah I switched to this version to address your comment here. And yeah I think this is better that we don't violate assumptions on TEEs and keep ExplicitLocals to its core tasks, while we put all ugly things in fixUnwindMismatches, which is ugly anyway :$... Joking, but I think it's better that fixUnwindMismatches takes care of its own messes.

• comment fix

Closing in favor of D81851.

(Phabricator tip: You can set parent / child revisions, which can be useful in case multiple CLs are in flight and have not been landed yet :) Probably you already know that, but anyway just in case)

• is -> gets / becomes

In D81758#2090825, @dschuff wrote:

(I'll use the term "de-stackify" as a verb to avoid confusion).

Some lines in WebAssemblyInstrAtomics.td and WebAssemblyInstrSIMD.td look they exceed 80 cols, so please fix that too :)

• Fix typo in comment

LGTM either with isNotDuplicable or isConvergent, as long as it works. I'm not familiar with what isConvergent is used for either, so..

Question: Is this feature gonna be handled by -mtriple and not -mattrs like other features?

LGTM.
Nit: NFC usually means code change that does not affect output, so adding more tests is not NFC by that definition.

I see. Thanks for the explanation.

In D81055#2072399, @dschuff wrote:

Binaryen doesn't assume an unreachable is actually unreachable, the way C compilers do, does it? (and remove code before the unreachable)? I think it just knows that instructions that come after it are unreachable and removes those. I think it's ok once it's lowered to wasm unreachable; it really won't be able to return (and so Binaryen can do DCE), but that seems like a legit behavior for @llvm.debugtrap according to the langref.

I'm not very familiar with how wasm debugger works, but do they use unreachable for breakpoints? Binaryen assumes unreachable is really unreachable and does all sorts of optimizations including DCE and everything, so if unreachable is actually used in debugger, that seems risky..?

I didn't know about this optimization opportunity! Please make sure to run emscripten tests before landing because it contains some new assumptions.