User Details
- User Since
- Jul 31 2018, 10:54 AM (204 w, 6 d)
Wed, Jun 29
Nice! LGTM with those comments.
Mon, Jun 27
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).
Wed, Jun 22
Tue, Jun 21
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!
Wed, Jun 8
Tue, Jun 7
- dot_add takes three operands
Mon, Jun 6
May 23 2022
Thanks, this makes more sense now. LGTM with fixes and extra information in that comment.
May 20 2022
May 16 2022
May 10 2022
LGTM! (with single comment)
Apr 22 2022
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.
Apr 21 2022
Apr 11 2022
No concerns about the WebAssembly changes. Will the default installation for folks who don't touch LLVM_DISTRIBUTION_COMPONENTS change?
Mar 22 2022
Mar 17 2022
Great, thanks @jhenderson!
Mar 16 2022
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
Mar 15 2022
Right now (after trying to upgrade my gdb) I get this error
Mar 14 2022
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?
Mar 7 2022
Mar 4 2022
@dyung, this should be fixed by https://reviews.llvm.org/rGc01ec3083026f7e24e6c06f48a05d413e2c697d4
This broke the build, working on a fix now.
Feb 17 2022
Feb 15 2022
Feb 7 2022
Jan 31 2022
Jan 27 2022
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?
Jan 20 2022
Jan 18 2022
Jan 17 2022
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?
Jan 12 2022
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.
Dec 15 2021
Dec 14 2021
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.
Dec 9 2021
@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.
Dec 6 2021
LGTM with the extra suggested test, if possible.
Dec 3 2021
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.
Nov 30 2021
WebAssembly test changes lgtm 👍
Nov 14 2021
Nov 8 2021
Nov 5 2021
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.
Nov 2 2021
Oct 28 2021
"globala" => "globals" in the descriptions (although globala sounds like a great name for something).
LGTM, thanks!
Oct 27 2021
Oh yeah, I guess we could only do this if bulk memory is enabled.
Code LGTM, thanks!
Oct 26 2021
Oct 24 2021
Thanks!
Oct 20 2021
LGTM!
Oct 19 2021
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.