Project

General

Profile

Bug #2562

Clarify how to do float->int rounding

Added by Roland Schulz 21 days ago. Updated 16 days ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
-
Target version:
-
Affected version - extra info:
Affected version:
Difficulty:
uncategorized
Close

Description

In most places we round float->int using (int)f+0.5. That has multiple problems:
- incorrect for 0.5-eps (gives 1 instead of 0)
- incorrect for negative numbers (without any check/assert and easy to miss in code review - I suspect multiple real issues in code)
- unnecessary slow (compared to some possible solutions)
- inconsistent with rounding used for float->float (round half away from zero vs round half to even)

What should the default rounding mode be?
- Round half up: Convention usually used outside of computers. Inefficient. No C++ function.
- Round half away from zero: Matches lround(). Somewhat efficient.
- Round half to even: Matches fp rounding and lrint(). Most efficient.

Possible solutions:
1) Round half away from zero with fast option: Create fastround() for performance critical code for positive inputs. It assert that the input is positive and uses +.5 otherwise. Document that it is wrong for 0.5-eps. Use lround/lroundf for non performance critical code.
2) Round half away from zero always correct: Always use lround/lroundf.
3) Round half to even: Always use lrint
4) Mix modes. Use lrint for performance critical code or where consistency with fp rounding is better. Use lround/lroundf otherwise.

History

#1 Updated by Eric Irrgang 20 days ago

Roland Schulz wrote:

4) Mix modes. Use lrint for performance critical code or where consistency with fp rounding is better. Use lround/lroundf otherwise.

It seems like the default should be that the function does what it says it does, but that fast routines should be available. Doesn't something similar come up for evaluating exponentials? Should the developer pick and choose which version they want for each math operation used with explicitly incompatible signatures or should they just toggle all implementations by using either `math` or `fastmath` with the same API? I'm inclined to the latter. For one thing, it would make for easier testing of whether performance or precision were more important in a given translation unit or even leave it to the user to decide with a CMake option.

#2 Updated by Roland Schulz 20 days ago

I agree clarity is the most important thing. And the current approach of static_cast<int>(x+0.5) isn't clear at all.
Do you mean we shouldn't use the standard rounding functions directly but have a gmx_round function which can be called both as gmx_round(x) and gmx_round<MathOptimization::Unsafe>(x)? gmx_round would call lround/lroundf I assume? What would gmx_round<MathOptimization::Unsafe> do? Would it use static_cast<int>(x+0.5) or lrint?
Note that this isn't just about safe and unsafe. I would consider lrint safe. It never gives incorrect results. But it uses a different rounding model which can be better or worse depending on the use case.

I think static_cast<int>(x+0.5) has no advantage over lrint. It is inaccurate and slower.
Maybe we should have
gmx_round<FastToEven>: Uses even rounding and is fast. Calls rlrint
gmx_round<SlowToZero>: Uses to zero rounding. Call lround/lroundf

Not sure which of the to should be the default (or whether there should be default). But would be clearer than using lround/lrint because those names are not very clear. Disadvantage is that it is non-std and thus unfamiliar to people new to GROMACS. Also it would be very verbose especially if we choose not to have a default.

#3 Updated by Berk Hess 20 days ago

Doesn't lrint cause overhead because it returns a long, while we nearly always use int?

I think we can use gmx_round to round to nearest, fast.

Do we ever need round to zero for negative numbers? If not, we can simply use static_cast for (positive) numbers, or?

#4 Updated by Eric Irrgang 19 days ago

Roland Schulz wrote:

gmx_round<FastToEven>: Uses even rounding and is fast. Calls rlrint
gmx_round<SlowToZero>: Uses to zero rounding. Call lround/lroundf

On a related note, what are the thoughts on how to make such a helper easily discoverable in the documentation. It seems like the availability and encouragement to use such functions should be within a couple of clicks of http://manual.gromacs.org/documentation/current/dev-manual/index.html but I don't see an appropriate place to stick it. Clicking through the Doxygen documentation, it is hard to find conventional GROMACS alternatives even if you know to look for them. (In the "library" documentation build, you can click through and discover the "math" header directory, but most of the files don't expose documentation there.)

My question is just: how strongly developers would be encouraged to use such functions and what documentation, if any, would support that intention, or whether this would be just a quietly internally documented function for use in a handful of cherry-picked places?

#5 Updated by Roland Schulz 18 days ago

I think the best enforcement is clang-tidy. That way we don't have to catch things in code-review and no one has to read+remember it. The reason I looked at it was part of my clang-tidy effort. It has a check for the (int)(x+0.5) pattern: http://releases.llvm.org/6.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/misc-incorrect-roundings.html. I think enabling it would be good. But of course before we enable it we need to decide what to replace it with. If we also want to have a strict rule to only use gmx_round and not lround/rint/... we could add a custom check for that.

#6 Updated by Roland Schulz 16 days ago

Doesn't lrint cause overhead because it returns a long, while we nearly always use int?

64bit shouldn't be an issue. Doing operations in 64bit should be as fast on supported HW (we don't support 32bit HW anymore) and 64bit->32bit conversion is free.

#include <cmath>
int f(float f) {
    return lrintf(f);
}

gets compiled to a single instruction in GCC (-O3 -ffast-math, >=4.81, X86-64). In comparison (int)(f+.5) is 3 instructions (float->double, add, trunc) and (int)(f+.5f) is 2 instructions (add, trunc). But other compilers are being stupid and turn this into a function call (but no extra instruction for 64->32). For llvm this is a known bug https://bugs.llvm.org/show_bug.cgi?id=22944 . Thus we probably would need to actually implement it ourselves if we wanted to not have performance regression on other compilers :(. Or use lrint for gcc and (int)(f+.5f) for other compilers.

Do we ever need round to zero for negative numbers? If not, we can simply use static_cast for (positive) numbers, or?

Looking at the warning given by clang-tidy misc-incorrect-roundings checks, there were multiple ones which looked suspicious and I assumed have a possibility to be negative. But I'm not sure. But I do think it is easy to accidentally get wrong.
I'm also not sure whether the error at 0.5-eps matters somewhere.

Also available in: Atom PDF