User Details
- User Since
- Jan 2 2013, 1:50 PM (496 w, 16 h)
Tue, Jul 5
Apr 12 2022
Mar 29 2022
lgtm
This looks good. I have just a couple of minor nits.
Mar 25 2022
@sepavloff I apologize for having lost track of this for so long. Do you have time to rebase this and the dependent patch?
I had forgotten that this patch never landed, but I was investigating a bug yesterday that I think this will help with (https://github.com/llvm/llvm-project/issues/48669).
Mar 22 2022
Mar 11 2022
This looks good to me. Thanks for the patch!
Mar 10 2022
This example illustrates the problem this patch intends to fix: https://godbolt.org/z/j445sxPMc
Mar 8 2022
The fix for the eval_method crash should be moved to a separate patch. Otherwise, this looks good. I have only minor comments.
Mar 7 2022
Mar 3 2022
Mar 2 2022
Feb 25 2022
Replacing __m128bh with __m128i does not prevent arithmetic operations on the type.
Feb 24 2022
Feb 23 2022
Jan 18 2022
lgtm
Dec 13 2021
Dec 3 2021
Nov 24 2021
Thanks for the patch! This looks mostly good. I have just a few suggestions.
Nov 2 2021
Nov 1 2021
Oct 28 2021
Sep 29 2021
Instead of generalizing the test, I re-ordered the checks to match what's currently happening. It appears to be stable and the test was broken by an explicit change to the object symbol ordering.
Sep 27 2021
Sep 7 2021
Aug 23 2021
Aug 5 2021
FWIW, fp-contract=on has been the documented default for clang since version 5.
Aug 4 2021
lgtm
Jul 29 2021
Jul 27 2021
Sorry for the non-response, I hadn't notice this review earlier.
Jul 26 2021
lgtm
Jun 30 2021
I think having a more robust library for FP comparisons in the test suite would be great. In fact, I thought about moving even the simple comparison I did in this patch to some common location where it could be included by other tests. Maybe that's a good starting point for an incremental change in the direction you're suggesting. A robust library of such checks is beyond the scope of what I have time to do at the moment, but I would certainly support that direction.
Jun 25 2021
Jun 10 2021
May 21 2021
This seems OK as a short term solution, but it is also problematic in that it prevents FMA from being used in these performance measurements. I understand that it's not trivial to allow FP-related error tolerance in the test, but (as a future patch) it would be nice to at least have a way to turn off the exact result checking on FP tests so that the performance could be measured with fast-math and fp-contract enabled, and that would require having a way to re-enable FMA for these tests.
May 10 2021
Apr 27 2021
Apr 23 2021
Apr 6 2021
Mar 1 2021
I'm not trying to be difficult, but I genuinely still don't understand the additional arguments pointer. Is it intended to allow proprietary extensions? Is there an example somewhere?
Feb 22 2021
Addressed review feedback
Ping
Feb 17 2021
Removed unnecessary bitcast
Feb 16 2021
Feb 12 2021
@pcc CODE_OWNERS.txt says that you own the bitcode. Can you help with this review?
Feb 3 2021
Jun 12 2020
lgtm
Jun 9 2020
Sorry for having let this drop for so long. Some other priorities came up, but I am still interested in seeing this through.
May 19 2020
May 13 2020
May 12 2020
I don't understand why this difference exists. Beyond just trying to reproduce what MSVC does, can you explain the difference?
Apr 22 2020
Mar 30 2020
Mar 26 2020
Sorry for the delay in approval.
Mar 12 2020
OK. Since the behavior for out-of-range evl is target-dependent, undefined makes sense.
Mar 11 2020
Can you use the "Edit Related Revisions" link to set the parent/child relationships of these patches and put "[1/5]" in the titles?
What is the intention of this set of patches in relation to D75938? It was unclear to me whether you intended to commit this implementation or were just offering it as an alternative for discussion.
I'm satisfied with the functionality, but I'm not sure about the intrinsics having undefined behavior outside the [0, W] range. The way you've implemented it, it seems like the behavior would be predictable. If the evl argument is outside that range, it is ignored. Applying an unsigned value greater than W using the "%mask AND %EVLmask" also has this effect. Why not just make that the defined behavior?
Mar 10 2020
Mar 4 2020
lgtm
Feb 27 2020
Feb 25 2020
I may have overstated. The exception behavior of the
Feb 24 2020
Any more feedback? Should I proceed in this direction?
Feb 21 2020
I don't know enough about PowerPC to review the check in the tests, but the basic constrained intrinsic handling looks correct.
It's a little weird that we're still recognizing these as LibFuncs, but I guess that's necessary to keep the optimizations like constant folding working with them. I see that ConstantFolding already calls the non-finite-restricted versions of the functions when it's doing compile-time evaluation. It would be good to have a comment somewhere (probably in TargetLibraryInfo.cpp below the note about math-finite.h) explaining the handling of these functions.