-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Fixed clang -Ofast issue. #9409
base: master
Are you sure you want to change the base?
Conversation
CMakeLists.txt
Outdated
@@ -346,7 +346,7 @@ endif() | |||
if(WIN32 OR ARM OR PPC64LE OR PPC64 OR PPC) | |||
set(OPT_FLAGS_RELEASE "-O2") | |||
else() | |||
set(OPT_FLAGS_RELEASE "-Ofast") | |||
set(OPT_FLAGS_RELEASE "-O3") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang recommends to replace -Ofast
with -O3 -ffast-math
:
Users are advised to switch to
-O3 -ffast-math
if the use of non-standard math behavior is intended, and-O3
otherwise.
I don't think this PR should change the current behavior which enables fast math.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang recommends to replace
-Ofast
with-O3 -ffast-math
:Users are advised to switch to
-O3 -ffast-math
if the use of non-standard math behavior is intended, and-O3
otherwise.I don't think this PR should change the current behavior which enables fast math.
I see, I noticed that there is a conflicting opinion between SChernykh and 0xFFFC0000.
As of now, I have removed the lines as suggested by 0xFFFC0000. Should I add the flag -ffast-math instead?
Imho, if we remove the following lines we are at better shape:
CMake will add Regarding |
Hi, removed thes lines. I noticed that there is a conflicting opinion between SChernykh and 0xFFFC0000. As of now, I have removed the lines as suggested by 0xFFFC0000. Should I add the flag -ffast-math instead? |
My comment was only regarding the "equivalence" of @0xFFFC0000 why don't you like |
@SChernykh Usually To clarify, I am more hesitant about About If I remember correctly most open source projects use
In some cases Overall, let me emphasize it again, I can live with
|
It's a slippery slope to abandon -O3 just because there might be an undefined behavior somewhere. UB is a bug and it should be fixed. It's better to run UBSAN as a part of unit tests. |
As for Edit: and in wallet2's gamma picker. But even there, the difference in rounding caused by |
I agree, especially because users tend to be very sensitive to hash rates. Imagine, for example, that the graph above (labeled "Picture from figure 4") proves representative, and optimized builds boost daemon hash rates (or reduce overall daemon CPU usage) by 1%: Considering a single release build has a lot of downloads, and has a shelf life of 1-2 years, that 1% savings amounts to a huge win, despite the fact that the compilation takes a lot longer than -O2. |
I am repeating myself, but IMHO |
This is too broad statement and it lacks details. It's not unsafe per se, it just changes how floating point is implemented in a number of ways. The real difference between "normal" and "fast" math will be visible in a really edge cases: https://stackoverflow.com/questions/7420665/what-does-gccs-ffast-math-actually-do Consider also that all Monero code has been running with it enabled this whole time. TLDR: in 99.9% of cases, fast math will just result in different rounding of intermediate operations. As long as computation doesn't involve subtracting small values from huge values where relative rounding errors can be big, it's perfectly safe and will just give a slightly different result (a couple last bits in 64-bit double value will be different). |
First, I don't consider it a bug-bug, rather it's a different behavior under ffast-math. This was already mentioned in the link I posted https://stackoverflow.com/questions/7420665/what-does-gccs-ffast-math-actually-do
FTZ and DAZ flags are totally fine for 99.9% of calculations, and I don't know of any place in Monero code where denormals are used. So my post still stands. |
Or another example: https://twitter.com/kwalfridsson/status/1450556903994675205 There are just too many moving parts with
We have random test failure there which I suspect caused by Having |
I would say using floating point at all is walking on landmine. It's fine in non-critical parts of the code, and critical code must use integers (and unsigned ones at that) - that includes the wallet2's gamma picker. Fast math or no fast math doesn't matter if it's just used for display and logs, and compiler bugs can happen anywhere, not just with fast math. I would rather increase test coverage and run tests also with UBSAN to feel safer, than simply disabling fast math and be like "yeah it'll be fine". |
Another one: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93806 to be fair. |
My proposal is even if we are going to use |
To be fair to |
Fixed issue clang compiler flag -Ofast [Deprecating -Ofast Issue #9407]