Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reenable fma #56

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Reenable fma #56

wants to merge 1 commit into from

Conversation

NAThompson
Copy link

No description provided.

@NAThompson
Copy link
Author

NAThompson commented Jun 1, 2023

@mborland, @jzmaddock, @swatanabe : Could your review?

Couldn't figure out how to get this one to compile:

BOOST_TEST(bu::fma(L1, 1.0/L1, 1.0) == 2.0);

but . . . perfect is the enemy of the good.

@mborland
Copy link
Member

mborland commented Jun 1, 2023

It seems reasonable to me. I think something of note if it's not already known is this transitively requires C++14 since cmath.hpp includes a number of headers from Boost.Math.

@jzmaddock
Copy link
Contributor

If you wanted to handle all of the mixed argument cases (there are a lot!), then the only sensible way to do it is to provide one fma template that is enable_if'd on at least one of the template parameters being a unit. Then in the body you need to call an "unwrap" helper function that returns the result of argN.value() when argN is a unit, and simply argN otherwise. Hope that all makes sense!

@swatanabe
Copy link
Contributor

swatanabe commented Jun 2, 2023 via email

@NAThompson
Copy link
Author

NAThompson commented Jun 2, 2023

The other mixed argument cases are not necessary.

These were added due to a compile error I observed on a Horner's algorithm where the abscissa is dimensionless and the coefficients are dimensioned. The compile error when only enabling the original overload is as follows:

In file included from ~/test/polynomial_test.cpp:22:
~/polynomial.hpp:58:11: fatal error: no matching function for call to 'fma'
      y = fma(y, x, c_[i]);
          ^~~
~/.conan2/p/boost21487c6284710/p/include/boost/units/cmath.hpp:242:1: note: candidate template ignored: could not match 'quantity<Unit2, Y>' against 'double'
fma BOOST_PREVENT_MACRO_SUBSTITUTION (const quantity<Unit1,Y>& q1,
^

Code:

  template<typename Real>
  [[nodiscard]] CoefficientType operator()(Real x) const noexcept {
    using std::fma;
    CoefficientType y = c_.back();
    long n = c_.size() - 1;
    for (long i = n - 1; i >= 0; --i) {
      y = fma(y, x, c_[i]);
    }
    return y;
  }

Compiler:

g++ --version
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: arm64-apple-darwin22.5.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

@NAThompson
Copy link
Author

@swatanabe : Mind merging?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants