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

Use hardware FMA #3007

Merged
merged 9 commits into from
May 30, 2021
Merged

Use hardware FMA #3007

merged 9 commits into from
May 30, 2021

Conversation

eggrobin
Copy link
Member

…or Voigt-Kampff/Dekker if we don’t have that. Avoid std::fma.

#endif

// ⟦ab + c⟧.
inline double FusedMultiplyAdd(double a, double b, double c);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have tests for these functions to verify that we are calling the intrinsics correctly?

Sorry, something went wrong.

@@ -127,6 +135,7 @@ using internal_double_precision::Mod2π;
using internal_double_precision::TwoDifference;
using internal_double_precision::TwoProduct;
using internal_double_precision::TwoSum;
using internal_double_precision::VeltkampDekkerProduct;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would any client ever want to use this function? Shouldn't it be purely internal?

Sorry, something went wrong.

Copy link
Member Author

@eggrobin eggrobin May 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comments:

// Returns the exact product of its arguments. Note that this function checks
// whether |UseHardwareFMA| is true. If the value of that flag is already known
// from context, it may be preferable to either:
// — use VeltkampDekkerProduct(a, b) below;

@pleroy
Copy link
Member

pleroy commented May 30, 2021

You've added references to a test file in the projects, but not the test file itself.

@pleroy
Copy link
Member

pleroy commented May 30, 2021

Also, why don't we change cpuid.cpp into cpuid_body.hpp? That would avoid adding the cpp files to all the projects, and it seems to make sense to inline these operations anyway.

@eggrobin
Copy link
Member Author

It doesn’t really make sense to inline that ; we don’t want to do CPUID calls every time we use something, instead we do them once and refer to the static result (here UseHardwareFMA).


using base::CPUFeatureFlags;
using base::HasCPUFeatures;
using ::testing::Test;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally don't do a using of this class, we use the full name when declaring the fixture. Also, we put the using of symbols that start with :: at the end of the list.

@pleroy pleroy added the LGTM label May 30, 2021
@eggrobin eggrobin merged commit 9b518f6 into mockingbirdnest:master May 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants