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.

Sign up for GitHub

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

Jump to bottom

Mark instruction helpers appropriately as constexpr #567

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Mark instruction helpers appropriately as constexpr #567

merged 1 commit into from
Oct 2, 2020

Conversation

axic
Copy link
Member

@axic axic commented Oct 1, 2020 •
edited
Loading

Pulled out of #562.

@axic axic requested a review from chfast October 1, 2020 14:48
axic
axic commented Oct 1, 2020
View reviewed changes
lib/fizzy/execute.cpp
@@ -423,7 +423,7 @@ __attribute__((no_sanitize("float-divide-by-zero"))) inline constexpr T fdiv(T a
}

template <typename T>
inline constexpr T fmin(T a, T b) noexcept
inline T fmin(T a, T b) noexcept
Copy link
Member Author

Choose a reason for hiding this comment

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

Both fmin and fmax use std::signbit which is not constexpr: https://en.cppreference.com/w/cpp/numeric/math/signbit

@axic
Copy link
Member Author

axic commented Oct 1, 2020

@chfast could you benchmark this?

@chfast
Copy link
Collaborator

chfast commented Oct 1, 2020

@chfast could you benchmark this?

Check assembly output first.

@axic
Copy link
Member Author

axic commented Oct 1, 2020

I don't see any change in any of the functions touched by this. Some stuff is moved around, so can't tell much with benchmarking.

@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

Merging #567 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #567   +/-   ##
=======================================
  Coverage   98.24%   98.24%           
=======================================
  Files          62       62           
  Lines        9048     9048           
=======================================
  Hits         8889     8889           
  Misses        159      159

@chfast
Copy link
Collaborator

chfast commented Oct 1, 2020

I don't see any change in any of the functions touched by this. Some stuff is moved around, so can't tell much with benchmarking.

The execute.cpp.s is identical in both cases. As it should be.

@axic axic changed the title Mark instruction helpers appropriate as constexpr Mark instruction helpers appropriately as constexpr Oct 1, 2020
@axic
Copy link
Member Author

axic commented Oct 1, 2020 •
edited
Loading

I don't see any change in any of the functions touched by this. Some stuff is moved around, so can't tell much with benchmarking.

The execute.cpp.s is identical in both cases. As it should be.

It wasn't identical for me. Let me check again.

Still not identical, interestingly the resulting .S is the same number of characters, but the content is different.

@axic
Copy link
Member Author

axic commented Oct 2, 2020

I don't see any change in any of the functions touched by this. Some stuff is moved around, so can't tell much with benchmarking.

The execute.cpp.s is identical in both cases. As it should be.

It wasn't identical for me. Let me check again.

Still not identical, interestingly the resulting .S is the same number of characters, but the content is different.

I totally forgot to consider optimiser settings when comparing the output. With the settings of the release build indeed it is identical.

@axic axic merged commit c676f64 into master Oct 2, 2020
@axic axic deleted the constexpr branch October 2, 2020 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chfast chfast chfast approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@axic @chfast