Page MenuHomePhabricator

[Support] Add mathematical constants
ClosedPublic

Authored by evandro on Mon, Sep 30, 9:08 PM.

Details

Summary

Add own version of the mathematical constants from the upcoming C++20 std::numbers.

Diff Detail

Event Timeline

evandro created this revision.Mon, Sep 30, 9:08 PM
evandro updated this revision to Diff 222658.Tue, Oct 1, 11:21 AM

Split the changes to the AMDGPU target in a separate patch.

efriedma added inline comments.Tue, Oct 1, 3:38 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5003

I don't like taking a 64-bit constant and truncating it to 32 bits. For most common mathematical constants, it probably produces the right result, but it's not correctly rounded in general.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1943–1944

Do we actually have enough precision here if the type is long double?

Maybe take std::numbers implementation - llvm::numbers?

evandro marked 2 inline comments as done.Wed, Oct 2, 10:33 AM

Maybe take std::numbers implementation - llvm::numbers?

Please, elaborate.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5003

Suggestions?

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1943–1944

The method is defined as static Constant *get(Type* Ty, double V). Likewise, the POSIX constant M_E is double precision only.

efriedma added inline comments.Wed, Oct 2, 3:57 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5003

You can have two constants: ln2 and ln2f, which are double and float respectively.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1943–1944

I guess this isn't making the existing problem worse, but please leave a FIXME, at least.

efriedma added inline comments.Wed, Oct 2, 4:02 PM
llvm/include/llvm/Support/MathExtras.h
69

Please use the correct number of digits (the smallest number of digits required to produce the correct double-precision result). Adding extra digits which don't actually affect the value is confusing.

evandro updated this revision to Diff 223373.Sat, Oct 5, 9:51 AM
evandro marked 4 inline comments as done.
evandro marked an inline comment as done.Mon, Oct 7, 12:40 AM
evandro updated this revision to Diff 223635.Mon, Oct 7, 12:24 PM
hiraditya accepted this revision.Mon, Oct 7, 2:45 PM

LGTM as the comments have been addressed.

This revision is now accepted and ready to land.Mon, Oct 7, 2:45 PM
efriedma requested changes to this revision.Mon, Oct 7, 2:54 PM
efriedma added inline comments.
llvm/include/llvm/Support/MathExtras.h
66

The correct value of sqrt(2) in double-precision is 1.4142135623730951.

And now I don't trust any of the other values...

71

Please mark the constants with "f", e.g. 2.718282f, so there isn't an extra rounding step.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1944

I'm not sure this describes the issue correctly. You can specify a long double as a string, or raw bits. It can't interoperate with the native long double because that might have the wrong width.

This revision now requires changes to proceed.Mon, Oct 7, 2:54 PM
evandro marked 3 inline comments as done.Mon, Oct 7, 3:00 PM
evandro added inline comments.
llvm/include/llvm/Support/MathExtras.h
66

double has a precision of 15 or 16 significant digits. I don't understand why are you suggesting 17 significant digits when you asked to trim the precision down.

Besides, the reference I provided states that this value is 1.41421356237309505. Whether it's rounded to 1.4142135623730950 or 1.4142135623730951 is a bit moot, IMO.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1944

What wording would you suggest, please?

evandro updated this revision to Diff 223671.Mon, Oct 7, 3:08 PM
efriedma added inline comments.Mon, Oct 7, 3:28 PM
llvm/include/llvm/Support/MathExtras.h
66

I asked for "the smallest number of digits required to produce the correct double-precision result". This is what you get if, for example, you ask Python 2.7 or later to convert the value to a string with repr() (printf "import math\nprint(repr(math.sqrt(2)))" | python). 1.414213562373095 produces a value that's different by one ulp.

Yes, a one ulp difference is unlikely to matter for most uses, but if we're going to take the time to define these, we should define them correctly.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1944

Maybe just FIXME: add more precise value of "e" for various long double types.

evandro marked 3 inline comments as done.Tue, Oct 8, 8:49 AM
evandro added inline comments.
llvm/include/llvm/Support/MathExtras.h
66

You're assuming that Python is correct. bc says 1.41421356237309504880. glibc's math.h says 1.41421356237309504880 as well. And none of these is the same as your 1.4142135623730951.

As I said, the precision of double is 15 to 16 digits and of float, 6 to 7 digits. math.h defines them with 20 digits, which is probably an agreeable precision, yes? But I believe that we call all live with a difference of ±1ulp.

evandro updated this revision to Diff 223915.Tue, Oct 8, 11:54 AM
evandro marked 2 inline comments as done.

@efriedma, you had a point. I now trimmed the precision down to one digit short of seeing a change in the mantissa bits.

evandro updated this revision to Diff 223925.Tue, Oct 8, 12:54 PM

Added the hex FP constants for easier auditing.

efriedma accepted this revision.Tue, Oct 8, 4:55 PM

LGTM

llvm/include/llvm/Support/MathExtras.h
66

1.4142135623730951 is the shortest decimal representation that produces the same double-precision number as 0x1.6a09e667f3bcdP+0. It isn't "correct" in any other sense, sure.

A few extra digits is okay, I guess.

This revision is now accepted and ready to land.Tue, Oct 8, 4:55 PM
evandro marked 2 inline comments as done.Wed, Oct 9, 12:23 PM
evandro added inline comments.
llvm/include/llvm/Support/MathExtras.h
66

Indeed, for some numbers more digits were necessary than for others. For uniformity's sake, I used the maximum from the number of digits required.

Thank you.

This revision was automatically updated to reflect the committed changes.
evandro marked an inline comment as done.