Page MenuHomePhabricator

[libc] Add implementation of fmaf.
ClosedPublic

Authored by lntue on Jan 4 2021, 10:15 AM.

Diff Detail

Event Timeline

lntue created this revision.Jan 4 2021, 10:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2021, 10:15 AM
lntue requested review of this revision.Jan 4 2021, 10:15 AM
lntue updated this revision to Diff 314406.Jan 4 2021, 10:18 AM

[trivial] Fix test comment.

Harbormaster completed remote builds in B83912: Diff 314404.
lntue updated this revision to Diff 314510.Jan 4 2021, 11:37 PM

Remove unused local variables.

cheng.w added a subscriber: cheng.w.Jan 5 2021, 2:54 AM
cheng.w added inline comments.
libc/test/src/math/FmaTest.h
2

Nit: Maybe replace the second space after fma with -.

libc/test/src/math/fmaf_test.cpp
2

Nit: Maybe replace the second space after fmaf with -.

libc/utils/MPFRWrapper/MPFRUtils.h
60

Nit: maybe "Operations which take ..."

lntue updated this revision to Diff 314593.Jan 5 2021, 6:50 AM

Fix comment typo.

lntue marked 3 inline comments as done.Jan 5 2021, 6:50 AM
sivachandra added inline comments.Jan 5 2021, 11:57 AM
libc/src/math/fmaf.cpp
23

Nit: Can we use z directly?

25

Can you add comments explaining why this correction is required?

libc/utils/FPUtil/FPBits.h
89

This will not be true for long double on i386. Will replacing it with an inequality be OK?

sizeof(XType) <= sizeof(UIntType)
libc/utils/MPFRWrapper/MPFRUtils.cpp
39

Is this used anywhere?

42

Does it make sense to put these values in FPBits?

363

Can you add a comment explaining why we can't use a single large precision for all types?

lntue marked an inline comment as done.Jan 5 2021, 11:15 PM
lntue added inline comments.
libc/src/math/fmaf.cpp
23

Change to use bitz directly (which is already in double) to reduce the number of float -> double conversion needed (just in case compiler does not optimize it).

libc/utils/FPUtil/FPBits.h
89

the (in)equality only apply when XType is of Integral type, and we were expecting XType == UIntType even when T is long double. That case was specialized before. When T is long double on i386, we still define FPBits<long double>::UIntType=__ uint128_t and the previously specialized constructor becomes FPBits<long double>(__uint128_t), so I think equality works correctly.

Changing it to inequality, we will allow extra constructors such as FPBits<long double>(uint32_t), which will not be what we want.

lntue updated this revision to Diff 314809.Jan 5 2021, 11:17 PM

Add comments explaining the correction part.

sivachandra accepted this revision.Jan 6 2021, 9:23 AM
sivachandra added inline comments.
libc/src/math/fmaf.cpp
22

What I meant was, instead of using the conversion operator, we can do something like:

fputil::FPBits<double> bit_sum(static_cast<double>(bit_prod) + static_cast<double>(z));

This avoids calling FPBits conversion operator. Likewise with bit_prod may be? That is, store the product in a double value instead of FPBits object, and use it to calculate the sum.

double prod = static_cast<double>(x) * static_cast<double>(y);
fputil::FPBits<double> bit_prod(prod);
fputil::FPBits<double> bitz(static_cast<double>(z));
fputil::FPBits<double> bit_sum(prod + static_cast<double>(z));
libc/utils/FPUtil/FPBits.h
89

Ah, sorry I misread! Ignore my original comment.

libc/utils/MPFRWrapper/MPFRUtils.cpp
363

Did you add any comment for this somewhere?

This revision is now accepted and ready to land.Jan 6 2021, 9:23 AM
lntue updated this revision to Diff 314976.Jan 6 2021, 1:43 PM
lntue marked an inline comment as done.

Address comments.

lntue updated this revision to Diff 314977.Jan 6 2021, 1:48 PM

Add comments.

lntue marked 2 inline comments as done.Jan 6 2021, 1:49 PM
lntue added inline comments.
libc/utils/MPFRWrapper/MPFRUtils.cpp
39

No it's not. Removed the default value.

42

Yes, it should stay together with MantissaWidth and other constants For now this is the only place using it so it does not matter much. We can move it when cleaning up / consolidating FPBits infrastructures.

This revision was landed with ongoing or failed builds.Jan 6 2021, 2:15 PM
This revision was automatically updated to reflect the committed changes.