- User Since
- Jun 10 2014, 9:32 AM (171 w, 3 d)
Wed, Sep 13
Tue, Sep 12
Wed, Sep 6
LGTM. Sorry that I didn't see this earlier.
Aug 21 2017
... except, please add another test-case where the other component is not an integer as well.
This looks fine.
Aug 4 2017
Mar 31 2017
(x-y) == 0 --> x == y does not require nsz (zeros of any sign compare equal), nor does it require nnan (if x or y is NaN, both comparisons are false). It *does* require ninf (because inf-inf = NaN), and it also requires that subnormals are not flushed by the CPU. There's been some churn around flushing recently, Hal may have thoughts (+Hal).
This addresses my concerns. Gentle ping to the other reviewers.
Mar 21 2017
The assumption is that an integer add is more canonical and/or cheaper than an FP add. Is that universally true?
Mar 8 2017
What's the rationale for using rationals here instead of a (simpler) fixed-point representation, if we want to get rid of float?
Feb 7 2017
Great, thanks Simon! This looks fine numerically. @gottesmm, can you double-check for style, etc?
Feb 1 2017
Numerically this is sound, and something that I've wanted to fix for a while but haven't had time to deal with. Thanks for doing it! Please add some test cases that would detect the old error (@gottesmm can likely point you in the right direction).
Jan 19 2017
These tests should either be exact, or should have a tolerance that's mathematically sound. +/-1ulp is not sound; the allowed error should be proportional to the magnitude of the larger of the real and imaginary components of the result -- e.g. if one component is very small compared to the other, the smaller one may have *no* "correct" bits and still be acceptable.
Jan 6 2017
OK, I'm satisfied with that.
Jan 4 2017
Dec 23 2016
I don't particularly care about the shift, since the code is completely equivalent either way, though it would be nice to clean up. However, please replace "mantissa" with "significand" before committing.
Dec 22 2016
Please s/mantissa/significand/. I know that "mantissa" is used in a number of places in llvm sources, but it's incorrect terminology. Significand is the IEEE-754 nomenclature, which any new work should follow.
Sep 26 2016
The tests in question are broken. They blindly apply a relative error tolerance that is not motivated by sound numerical analysis, despite the existence of well-known and well-founded error bounds for these problems, in every basic numerical analysis textbook.
Sep 22 2016
Sep 12 2016
Abe, I had a patch to do the same thing last year (that we ended up backing out because it exposed a bug which has since been fixed). Your approach looks a bit different from the one that I took originally, and you seem to be missing some of the arm64 test changes that I had to put in:
Jul 15 2016
I am not the right person to review the C++ template details, but everything else seems OK to me.
Jul 1 2016
I agree with Marshall. Aside from the points he raises, this approach looks fine.
May 26 2016
Numerically, I'm fine with this. Someone else can address the approach and style, etc.
May 23 2016
May 17 2016
-ffp-contract=on obeys the semantics of C's FP_CONTRACT pragma. In particular, it will not fuse:
float m = x*y; float a = m + z;
Whereas you probably want that to fuse for your purposes. -ffp-contract=fast seems more in line with your needs.
May 12 2016
Apr 8 2016
The input code is denorm preserving on architectures that flush denorms, but the output may not be.
There are a few niche floating-point formats that use a representation of the form [ exponent | 2s complement significand ], so the signbit ends up sitting in the middle of the number. Does LLVM claim to support arbitrary oddball formats for float/double? Ideally we would specify that LLVM assumes IEEE formats, even if we don't require fully conformant operations in hardware; that would give us some flexibility.
Apr 7 2016
Breaking isnan, isinf, etc is a non-starter. I know it's appealing from a consistent formal model viewpoint, but in practice it breaks a lot of code (this is why we have the call fallbacks for iOS/OSX).
Mar 22 2016
OK. Let's take this in as is, then, and someone can look at normalize( ) when they have time.
Mar 21 2016
Mar 15 2016
Mar 12 2016
Jan 29 2016
The signbit of NaN explicitly has no meaning, so there's no concern there. LGTM.
Jan 11 2016
Dec 4 2015
Dec 2 2015
Abandoned for D15165.
This is mostly http://reviews.llvm.org/D14891 with a test case added, but D14891 also fixed a second very minor issue: that the "else if" should just be "if". Also, majnemer made a few style suggestions there that it would be nice to adopt. Either way, we should merge the two patches. This can be the canonical one if you want to update it.
Nov 30 2015
As updated, this seems fine to me.
Nov 28 2015
Could this be resolved by using -0 as the constant instead of 0 in non-fast-math mode?
Something that also probably needs to be thought about is what will be the default behavior for clang and how to control it?
Nov 20 2015
Nov 18 2015
As long as we're really going to dig into the addition-chain issue, why are we duplicating Reassociate::buildMinimalMultiplyDAG here?
Nov 17 2015
This looks good to me now. One of the owners should probably sign off on it too.
Hence the "something like". We should really just be able to pull and integer value out of APFloat (but again, outside the scope of this change). Short-term, I think you can use V.convert(APFloat::IEEEdouble, ...) to get something that *can* be converted to double.
Nov 16 2015
I went ahead and added isInteger( ) to APFloat (r253254). You can use that to simplify some of the logic and make it somewhat more general.
Nov 5 2015
The one thing that's a little bit weird here is cases like x = -1, y = 4; log(pow(-1, 4)) is 0, but 4*log(-1) is NaN. That's a dramatic difference even for fast-math. Do we find exact integer exponents before we get to this point?
Seems reasonable from a mathematical perspective.
Nov 4 2015
Additionally test contraction of compound assignment expressions.
Nov 3 2015
I'd like to see this made generic so it applies to any 1-to-1 libm function whose inverse is available, since the same pattern applies for logN(expN(x)), asinh(sinh(x)), atanh(tanh(x)), etc ...
Oct 30 2015
Seems reasonable to me. One of the owners should sign off on it.
Oct 26 2015
As suggested by David, this should be fast-math only. It's roughly equivalent to re-association of multiplication. Besides rounding differences, this changes overflow and underflow behavior quite dramatically. Consider x = 1000, y = 0.001. pow(exp(x), y) = pow(inf, 0.001) = inf, whereas exp(x*y) = exp(1).