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 Utilities::fixed_power() where possible. #16441

Merged
merged 2 commits into from Jan 16, 2024
Merged

Conversation

bangerth
Copy link
Member

@bangerth bangerth commented Jan 9, 2024

This addresses the majority of what #13321 is about. In particular, it replaces the expensive calls to std::pow() by Utilities::pow() if the exponent is known and an integer.

@drwells
Copy link
Member

drwells commented Jan 10, 2024

For the record - I think it's reasonable to do this for the sake of uniformity but every version of GCC I tested correctly expands std::pow(x, 2) to x * x at -O1 and higher. It expands higher powers only when -ffast-math is on:

https://www.godbolt.org/z/aEPbMd8ra

@blaisb
Copy link
Member

blaisb commented Jan 10, 2024

I still think it's a good idea to do this. It does not cost anything on our part and it ensures that it does what we think it should do. I like this PR.

Copy link
Member

@blaisb blaisb left a comment

Choose a reason for hiding this comment

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

Other than the comment @drwells pointed out. I think this is a good PR :)!

Comment on lines -4170 to -4172
std::sqrt(std::pow(nrml[i * d1 + j * d2](0), 2.) +
std::pow(nrml[i * d1 + j * d2](1), 2.) +
std::pow(nrml[i * d1 + j * d2](2), 2.));
Copy link
Member

Choose a reason for hiding this comment

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

@drwells I think this is an example where std::pow and Utilities::fixed_power will be different. std::pow(x,2.) does not expand to x*x as far as I remember trying. You need to have std::pow(x,2) with an integer. I think this PR makes this a lot clearer. I prefer expliciting the fixed_power.
<

Copy link
Member

Choose a reason for hiding this comment

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

Here, you could alternatively also use std::hypot (works in 3D since C++17).

Comment on lines 2457 to 2460
const double distance_to_camera =
std::sqrt(std::pow(point[0] - camera_position[0], 2.) +
std::pow(point[1] - camera_position[1], 2.) +
std::pow(point[2] - camera_position[2], 2.));
Copy link
Member

Choose a reason for hiding this comment

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

Also a good candidate for std::hypot.

Comment on lines -2628 to -2631
const double distance_to_camera = std::sqrt(
std::pow(point[0] - camera_position[0], 2.) +
std::pow(point[1] - camera_position[1], 2.) +
std::pow(point[2] - camera_position[2], 2.));
Copy link
Member

Choose a reason for hiding this comment

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

same here

@bangerth
Copy link
Member Author

Good points about the use of std::hypot. I did not know about this function.

@marcfehling
Copy link
Member

Technically, with #16455 in mind, we can also write these std::hypot lines as

double norm = nrml[i * d1 + j * d2].norm();

and

const double distance_to_camera = (point - camera_position).norm();

But let's handle that in a follow-up PR.

@marcfehling
Copy link
Member

As we decided against the use of hypot in #16455, can you delete the last commit 907d0f0 on this branch?

@bangerth
Copy link
Member Author

None of these are in performance critical places, and since it's easier (and shorter) to read, let's leave these in if you don't mind too much.

@marcfehling
Copy link
Member

@drwells If you give your okay for this PR we can merge it

@bangerth
Copy link
Member Author

@drwells Ping?

@kronbichler kronbichler merged commit 05ddc7c into dealii:master Jan 16, 2024
15 checks passed
@bangerth bangerth deleted the pow-2 branch January 16, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants