Looks good!

Should F and G be lowercased? It looks like the surrounding code uses camelCase fairly uniformly.

Thanks!

Discussed offline and decided that it would be better to error out in the linker and depend on the frontend to generate the necessary features based on flags like -mmutable-globals or -fPIC.

LGTM!

The status is that we decided to leave the behavior as-is despite the WebAssembly regression. I will (eventually) do a WebAssembly-specific fix, but there will be no need to backport that. No further action should be required here.

Yes, I only made this patch because the previous one was intended to be NFC. However, I agree (based on my cursory understanding of the relevant code) that it would be better to have just one code path, but this patch enforces an additional use of the second code path. It would make further cleanup of this code easier if I left this as-is and just fixed the issue in the WebAssembly backend, so I think I will abandon this revision and do that instead. @samparker and @aheejin, does that sound reasonable to you?

In D85759#2212136, @aheejin wrote:

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 :)

• Require feature subsets

In D85494#2214339, @aheejin wrote:

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?

Sounds good, I'll make that change. @alexcrichton, would it be useful to have a separate LLVM pass that applies all features used anywhere in a module to each function in that module? Frontends could run that pass early when compiling to WebAssembly to remove the inlining restrictions.

Thank you!

@sammax Yeah there's a lot of stuff going on there. Thinking about it more, I'm not sure we should be feature checking type signatures in the linker. We certainly aren't feature checking the contents of the linked functions. This patch LGTM, so I'll go ahead and land it as-is.

Nope, I guess we don't have any tests for this yet. @sammax, could you add a test in lld/test/wasm?

In D85759#2211133, @fhahn wrote:

In D85759#2211128, @tlively wrote:

Huh, the precommitted test failed on multiple bots even though it passes for me locally. I've reverted the commit that added the test for now while I investigate.

I think you have to add a REQUIRES for the WebAssembly target, otherwise bots that do not build that target will fail with the wasm32 triple.

In D85759#2211128, @tlively wrote:

Huh, the precommitted test failed on multiple bots even though it passes for me locally. I've reverted the commit that added the test for now while I investigate.

Huh, the precommitted test failed on multiple bots even though it passes for me locally. I've reverted the commit that added the test for now while I investigate.

• rebase on pre-committed test

@RKSimon Sure, I can do that.

Yes, most of the SIMD intrinsics could be folded as well. It’s certainly on my todo list.

In D85581#2204725, @sbc100 wrote:

"The issue was that an Address set to
be relative to the FrameIndex with offset zero was incorrectly
considered to have an unset base. "

From the patch I can't see how offset (which I assume means the Offset member) comes into play at all. It looks like the fix applies to when either the FI or the Ref itself is zero. Or should I read FrameIndex with offset zero to mean FrameIndex of zero ?

@alexcrichton As someone who works on an LLVM frontend, do you think the possibility that a feature can be "forgotten" would be worth introducing a restriction that functions can only be inlined into other functions that have a superset of their features?

• Fix and test macros as well

• Fix

• Renumber i32x4.dot_i16x8_s to match V8 as well

Nice! Thanks for doing that :)

It would be good to split this into (at least) two patches. The first should do the minimal testable amount of work to get instruction selection working, and follow-on patches can add the other parts, like additions to the object file format. Part of the reason for that split is that different people will be better at reviewing those different parts of the patch.

In D66035#2187021, @pmatos wrote:

In D66035#2181659, @pmatos wrote:

I will be splitting the part enabling the target feature through clang into a separate revision as suggested by @tlively

I just noticed that most of this work landed in an earlier commit

• Remove IntrSpeculatable

Since this changes opcodes, it needs to be landed in concert with the corresponding Binaryen change.

In D66035#2178105, @pmatos wrote:

Please ignore my .gitlab-ci.yml. That's just an internal change that I got uploaded by mistake.
I am looking to see this through and start discussion on this with the goal of landing it.

LGTM!

• Improve codegen: use faster i32x4.splats and reuse the existing vector as the second narrowing argument

In D84365#2168515, @aheejin wrote:

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?

I'm taking this patch sequence out of the review queue for now, pending an investigation of the alternatives suggested above.

Thank you all for the comments! I agree that the opcodes in SelectionDAG::isSplatValue are rather arbitrary, so a more principled approach using a TLI hook might be better. I will take a look at what X86 is doing to see if a simpler solution would work for WebAssembly, too.