Yeah adding a pass to propagate the superset of all functions features to all functions sounds reasonable to me, it would be easy enough for that to be added to the pass manager on a wasm-specific basis.

Nice find!

Ah that's actually a good point. I think in that case it may be best to only inline into functions which have a superset of features. A pretty common expected use is where you enable simd128 in some functions and then call a bunch of intrinsics. The standard library itself won't be compiled with simd128 by default, so that'll be the only way to use simd128.

Ok cool, thanks! I did test this out locally and it worked for use case, so on that front should be good to go!

Oh dear sorry I'm still quite green on submitting patches to LLVM, so I'm not actually entirely sure what the build failure here means. Is that a transient CI issue or something that I need to address? (searching around for "No valid crumb was included in the request" doesn't seem to turn up much...)

At least from my POV looks good :)

Seems reasonable to me! In my testing though if these existed as custom sections they'd still make their way to the final binary through LLD, so could LLD skip over these sectiosn by default?

I've tested this patch compiling the motivating test case for the original performance issue for Firefox and performance is back to what it was in LLVM 8, so I can at least say from my perspective that this does the trick!

Hello! We've discovered a regression in the Rust project from this commit, and I've opened a dedicated LLVM bug for that as well, and figured it'd be good to mention here as well!

Thanks for the tip! The test should now be deduplicated

Did you run the tests?

Fix tests

Er oops sorry haven't submitted too many patches before so was just using git diff, but I've tried an update with git diff -U100000 and that should have more context!

Updated diff with more context

er sorry, looks like some problems may already be known!

I think this has unfortunately caused a regression :( -- https://bugs.llvm.org/show_bug.cgi?id=39158

In some local tests it looks like this fixes the issue we were seeing, thanks so much!

I'm a bit late to this diff but I've bisected a regression in the Rust repo to this revision -- https://bugs.llvm.org/show_bug.cgi?id=38711

This is a bit late on this, but we're currently trying to upgrade LLVM in Rust and we've run into a performance regression bisected to this change, if those here familiar with this commit could help investigate that it'd be much appreciated!

I've bisected a recent regression we discovered in Rust to this commit and cc'd some of y'all on the bug there, but if anyone else would like to help take a look at the performance regression there that'd be much appreciated!

Oh no worries at all, thanks!

FWIW https://bugs.llvm.org/show_bug.cgi?id=37382 was bisected back to this revision which has shown up when we upgraded LLVM in the Rust project

Awesome, thanks for the quick fix!

Thanks! This looks like it would do the trick yeah. I think we've since removed the C++ code that caused the error on our end (for entirely unrelated reasons) so we're unlikely to hit this any more (and can probably revert the patch I had), but this'd be good to get into the next version of LLVM!

Wow that was fast, thanks so much! We'll be updating to this as soon as we can!

FWIW I've just recently started testing LLD support w/ Rust code again and LLD master is working beautifully! The output of lld was a little large given the set of exported items, but @sunfish pointed me at this diff which worked perfectly for us as well! This managed to strip notably all the unused entries in the global table for call_indirect entries.

FWIW I've tested this locally and it does indeed fix the issues we were seeing!

Thanks so much for helping us with this @JDevlieghere!

FWIW I've confirmed that this fixes the issue I was seeing and our entire test suite now passes, thanks again!

Oops sorry about that! Missed that last request, but should be taken care of now.

Ok thanks for the help! I've updated with some CHECK directives now. Mind helping me land the patch as well?

Sure thing, updated!

Ok, thanks for the info!

@chandlerc since we saw such a drastic decrease in compile times after including the original version of this patch, could you elaborate on the downsides of what would happen if we were to do that in our local fork of LLVM? Or would it still be unacceptable to send this patch upstream?