- User Since
- Aug 18 2015, 9:19 AM (271 w, 3 d)
Aug 13 2020
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.
Aug 9 2020
Aug 7 2020
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.
Apr 15 2020
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...)
Mar 31 2020
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?
Sep 16 2019
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!
Dec 16 2018
Nov 8 2018
Thanks for the tip! The test should now be deduplicated
Did you run the 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
Oct 2 2018
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
Aug 29 2018
In some local tests it looks like this fixes the issue we were seeing, thanks so much!
Aug 26 2018
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
Jul 6 2018
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!
Jul 3 2018
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!
Jul 2 2018
Oh no worries at all, thanks!
Jul 1 2018
May 29 2018
Awesome, thanks for the quick fix!
Mar 12 2018
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!
Jan 31 2018
Wow that was fast, thanks so much! We'll be updating to this as soon as we can!
Jan 30 2018
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.
Jan 24 2018
FWIW I've tested this locally and it does indeed fix the issues we were seeing!
Nov 13 2017
Thanks so much for helping us with this @JDevlieghere!
Nov 10 2017
FWIW I've confirmed that this fixes the issue I was seeing and our entire test suite now passes, thanks again!
Oct 10 2017
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?