User Details
- User Since
- Jul 31 2018, 10:54 AM (242 w, 5 d)
Wed, Mar 22
Yeah, I agree that many tests are better with smaller, manually written expectations. For these particular tests, the generated expectations aren't that long and are easy to scan for the expected instructions. Using the full output also helps show that the arguments, epilogues, and other surrounding code looks good as well.
Tue, Mar 21
Mon, Mar 20
Got it, thanks.
Oh oops, I had written a response but then I never clicked "submit." Sorry about that.
Thanks! I didn't know about this functionality. Is there a generally accepted mechanism to mark intrinsics as experimental so that there is no expectation that they will be auto-upgraded like this?
Code LGTM, although it looks like there may be some missing cases reported in https://github.com/llvm/llvm-project/issues/59095. Testing the end-to-end integration here might be a good thing to do in cross-project-tests.
Fri, Mar 17
LGTM besides using the test update script.
For the description, here's some possible clearer wording:
Wed, Mar 15
LGTM % language tweaks!
Wed, Mar 1
Great, thank you!
Tue, Feb 28
Thanks for the ping! Sorry this fell through the cracks.
Feb 21 2023
The clang driver will automatically call wasm-opt if it finds it in your PATH, so it's worth checking whether wasm-opt is being run or not.
Yes, pmin is rhs < lhs ? rhs : lhs and pmax is lhs < rhs ? rhs : lhs, so if either input is NaN, the result for either operation should be lhs. IIUC, before this patch NaN inputs would result in rhs instead.
Great, thank you!
Feb 17 2023
I don't think this is correct with respect to NaN vector elements. SIMDe is getting test failures starting with this commit, reported here: https://github.com/emscripten-core/emscripten/issues/18759
Feb 16 2023
LGTM, thanks!
Feb 6 2023
Impressive performance wins! How is code size affected?
Jan 13 2023
FWIW I had to patch the musl used by Emscripten to work around this: https://github.com/emscripten-core/emscripten/pull/18510
Jan 5 2023
Could you add a test showing the regression in a preliminary patch so that we can see the improvement in this patch?
Jan 3 2023
LGTM % comment. Thanks for taking this!
Nice!
LGTM
Dec 12 2022
Nice, it's good to use the builtin nodes where possible. I think we had intentionally selected splats of constants into constant vectors for performance reasons. Would you be able to preserve that behavior and make this a non-functional change?
Dec 9 2022
LGTM. It definitely makes the tablegen simpler, and I think the C++ isn't that complicated.
Dec 7 2022
Dec 6 2022
Nov 23 2022
Nov 21 2022
Nov 18 2022
- Fix type
Nov 17 2022
Nov 15 2022
LGTM, thanks!
Nov 14 2022
Nov 11 2022
I don't know if you've seen it, but we have a multivalue-stackify.py script that tries to exhaustively enumerate multivalue sequences up to a certain size then filter out the uninteresting ones. The output is checked in as the multivalue-stackify.ll test. It may be worth playing around with that script so that it generates programs that exhibit this problem.
Nov 7 2022
LGTM % comment. Thanks!
Oct 13 2022
It seems kind of strange to have features be free form strings rather than a set of flags, but maybe that's appropriate at this stage. It's certainly more flexible.
Oct 3 2022
Sep 26 2022
LGTM!
Sep 21 2022
Thanks! Do you need me to land this?
Sep 20 2022
Sep 13 2022
Yeah the unrelated test failures can be ignored. Good point about our shuffle combine workaround, @penzn. Would you have time and resources to investigate whether we can remove the workaround now?
Sep 12 2022
Sounds good. @fanchenkong1, it would be great if you could expand the comment as @penzn suggested. After that, do you have commit access or would you like me to merge this for you? If you would like me to commit it, what author email should I use for you?
Sep 9 2022
The code looks good, but I don't know much about native SIMD. Is this strictly better for engine-side codegen on all platforms? LGTM if so.
Sep 8 2022
Sep 7 2022
Aug 16 2022
It would be good to add tests for more error conditions like in the externref patch and also additional errors like trying to apply __funcref to types that aren't function pointers.
Very nice! This LGTM with all these small comments addressed. Sorry for the long delay in reviewing.
For reference, the existing error from the backend is something like this:
Oops, sorry about that. Will take a look.
Aug 3 2022
Jun 29 2022
Nice! LGTM with those comments.
Jun 27 2022
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).
Jun 22 2022
Jun 21 2022
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!
Jun 8 2022
Jun 7 2022
- dot_add takes three operands
Jun 6 2022
May 23 2022
Thanks, this makes more sense now. LGTM with fixes and extra information in that comment.