This is an archive of the discontinued LLVM Phabricator instance.

[libc] Implement double precision FMA for targets without FMA instructions.
ClosedPublic

Authored by lntue on Apr 26 2022, 6:41 PM.

Details

Summary

Implement double precision FMA (Fused Multiply-Add) for targets without
FMA instructions using __uint128_t to store the intermediate results.

Diff Detail

Event Timeline

lntue created this revision.Apr 26 2022, 6:41 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 26 2022, 6:41 PM
lntue requested review of this revision.Apr 26 2022, 6:41 PM
lntue updated this revision to Diff 425395.Apr 26 2022, 7:21 PM

Correct the sign of 0 when there is exact cancellation.

sivachandra added inline comments.Apr 27 2022, 5:36 PM
libc/test/src/math/fma_no_fma_test.cpp
11

You should be able to include generic/FMA.h directly and avoid this?

lntue added inline comments.Apr 27 2022, 7:24 PM
libc/test/src/math/fma_no_fma_test.cpp
11

The main point was to test that __llvm_libc::fma implementation works correctly without LIBC_TARGET_HAS_FMA. Unfortunately src.math.fma is an object target built with just 1 config (FMA flag on our machine) so I choose to inlining its implementation here, which is simply calling __llvm_libc::fputil::fma, regardless of FMA flags. If we include generic/FMA.h directly, we wouldn't know/test directly if the fma implementation in generic/FMA.h is called or __llvm_libc::fputil::fma works correctly without FMA flag, unless we run it on non-FMA machines.

I guess we could instrument the build system to do these, but I'm not sure if it could be cleaner, especially when the behaviors of such functions with or without FMA diverge.

sivachandra added inline comments.Apr 27 2022, 10:01 PM
libc/test/src/math/fma_no_fma_test.cpp
11

I think this config problem is now widespread enough that we don't want to set precedents like this. I agree that the plumbing from fputil/FMA.h to generic of specific implementations will not be tested if you directly use generic/FMA.h. But, I think testing your integer implementation should be of primary concern for this patch. We will solve the config problem separately.

zimmermann6 resigned from this revision.May 2 2022, 7:54 AM

I'm sorry, I not fluent enough in C++ to review this patch

lntue updated this revision to Diff 427957.May 8 2022, 3:16 PM

Rely on https://reviews.llvm.org/D125174 and https://reviews.llvm.org/D123615
for testing with and without FMA flags.

lntue marked an inline comment as done.May 8 2022, 3:18 PM
lntue added inline comments.
libc/test/src/math/fma_no_fma_test.cpp
11

With https://reviews.llvm.org/D125174 and https://reviews.llvm.org/D123615 , tests with and without fma flags are going to be generated automatically by the build system.

michaelrj accepted this revision.Jun 9 2022, 4:57 PM

overall, LGTM with a couple nits. Also, if you're going to add a clz header to FPUtil then please add a TODO for me to change str_to_float.h over to using it, or alternately change it yourself.

libc/src/__support/FPUtil/generic/FMA.h
153

nit: can you add something explaining why this number is 10?

183

nit: it should either be the sticky bits remain or the sticky bit remains

This revision is now accepted and ready to land.Jun 9 2022, 4:57 PM
lntue updated this revision to Diff 435936.Jun 10 2022, 8:59 AM
lntue marked an inline comment as done.

Address comments and add more tests.

lntue marked 2 inline comments as done.Jun 10 2022, 9:01 AM

overall, LGTM with a couple nits. Also, if you're going to add a clz header to FPUtil then please add a TODO for me to change str_to_float.h over to using it, or alternately change it yourself.

I updated str_to_float.h to use the same builtin_wrappers.

sivachandra accepted this revision.Jun 10 2022, 10:49 AM

Nit: The changes to sqrt*.cpp and str_to_float.h are not related - please land them separately as "Obvious".

lntue added a comment.Jun 10 2022, 5:39 PM

Nit: The changes to sqrt*.cpp and str_to_float.h are not related - please land them separately as "Obvious".

Refactor the changes in sqrt, str_to_float, and Hypot to https://reviews.llvm.org/rG2a746ebf1a4e