Addressing `-Wimplicit-float-conversion` warning might lead to precision loss

[firewave]

Disclaimer: My knowledge about floating-point precision is quite limited and I based most of my interpretations on the linked discussion. I came across this by accident and was wondering if there was something of the short-coming that could be detected by tooling. While looking into this I encountered this apparent issue.

This is based on the discussion in https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/28336.

According to that discussion the floating-point result of 48 * (1 / 255) should be 0.18823529779911041259765625. But if you simply write that it will lead to a compiler warning.

float f = 48 * (1.0 / 255.0);

warning: implicit conversion loses floating-point precision: 'double' to 'float' [-Wimplicit-float-conversion]
   12 |     float f = 48 * (1.0 / 255.0);
      |           ~   ~~~^~~~~~~~~~~~~~~

The obvious fix looks like adding the f suffix to the literals.

float f = 48 * (1.0f / 255.0f);

That fixes the warning but it changes the result to 0.1882353127002716064453125 which is supposedly less precise.

It took me quite a while until I finally came up with a solution which did not produce any warnings as well as the expected result.

float f = (float)(48 * (1.0 / 255.0));

As there are several ways to suppress (I do not consider it a fix when it changes the result/behavior) this warning seems likely to cause more harm than good so I thought it might be worth raising this issue.

See https://godbolt.org/z/crnKhzsec for all my attempts to mitigate this warning.

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
3. Fix the issue locally.
4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
5. Create a Git commit.
6. Run git clang-format HEAD~1 to format your changes.
7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Oliver Stöneberg (firewave)

Disclaimer: My knowledge about floating-point precision is quite limited and I based most of my interpretations on the linked discussion. I came across this by accident and was wondering if there was something of the short-coming that could be detected by tooling. While looking into this I encountered this apparent issue. This is based on the discussion in https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/28336. According to that discussion the floating-point result of `48 * (1 / 255)` should be `0.18823529779911041259765625`. But if you simply write that it will lead to a compiler warning. ```cpp float f = 48 * (1.0 / 255.0); ``` ``` warning: implicit conversion loses floating-point precision: 'double' to 'float' [-Wimplicit-float-conversion] 12 | float f = 48 * (1.0 / 255.0); | ~ ~~^~~~~~~~~~~~~~~ ``` The obvious fix looks like adding the `f` suffix to the literals. ```cpp float f = 48 * (1.0f / 255.0f); ``` That fixes the warning but it changes the result to `0.1882353127002716064453125` which is supposedly less precise. It took me quite a while until I finally came up with a solution which did not produce any warnings as well as the expected result. ```cpp float f = (float)(48 * (1.0 / 255.0)); ``` As there are several ways to suppress (I do not consider it a fix when it changes the result/behavior) this warning seems likely to cause more harm than good so I thought it might be worth raising this issue. See https://godbolt.org/z/crnKhzsec for all my attempts to mitigate this warning.

[erichkeane]

While the diagnostic text isn't perfect, it is telling you what is wrong: The expression on the RHS has a type of 'double', but you're storing it in a float via an implicit conversion, which loses precision. double has 64 bits of storage, and float has 32 on most systems.

A float can handle ~7 decimal digits accurately, and a double is about 15. Unlike integral types, the inaccuracy isn't just "short can hold only smaller magnitude values than int", it is "float can store fewer values everywhere on the number line than double".

The 'correct' actions here are: 1- Store the result in a double, rather than a float. So double f = 48 * (1.0f / 255.0f);. This has the advantage of losing no precision anywhere.

2- Do the calculations in the expression as floats. You did this with the f suffix. This loses precision at each step in the math (as the result of each sub-expression is done as a float at each step).

3- Cast the result of the expression to float. This does all the math as a double, then replaces the implicit cast with an explicit one, where you are acknowledging that you're losing precision in assigning to a float.

I'm not closing this, as there is perhaps value in a fixit diagnostic that shows where to put the cast to suppress the warning. This should also be an easy patch, so I'm marking it as 'good first issue', as adding a fixit should be sensibly easy.

For the person who decides to take on this issue: I'd expect C++ to suggest a static_cast<float>(<expr>), and C to suggest float(<expr>).

[firewave]

Thanks a lot for the detailed explanation.

In hindsight it does seem quite obvious. Also the "implicit" in the warning is a dead-giveaway that you should introduce an "explicit" cast to fix this properly. So a fix-it which does this would be a of much help.

But I think what was distracting me from the "obvious" fix are the visual cues in the warning:

   12 |     float f = 48 * (1.0 / 255.0);
      |           ~   ~~~^~~~~~~~~~~~~~~

This points to the * operator which seems to suggest that the warning is caused by the expression. But as you detailed the whole expression is fine and will have the double precision until the assignment. So I wonder if it should actually point to the = instead.

Looking at the other case in my examples:

   10 |     float f2 = (float)u * (1.0 / 255.0); // expected
      |                ^~~~~~~~ ~

Here it points to the cast suggesting that is the cause. But in this case it is caused by the expression thus I think it should point to the *.

[aidenfoxivey]

@firewave, are you looking to solve this issue? I'd like to take a crack at it if not.

[firewave]

@aidenfoxivey No, go ahead.

[aidenfoxivey]

@erichkeane

So the specific cast diagnostic I believe is here https://vscode.dev/github/llvm/llvm-project/blob/main/clang/lib/Sema/SemaChecking.cpp#L14576.

Is the desired behaviour that the warning warn_impcast_float_precision itself recommends float() || static_cast<float>() or that there is a separate warning for this specific case?

[aidenfoxivey]

I wrote it as such, but I'm unsure whether there's maybe a better way to approach it.

Naming wise I called them warn_impcast_float_precision and warn_impcast_float_precision_cpp, but I think that's perhaps a clumsy way to do it since I haven't seen any code approach it a similar way.

[aidenfoxivey]

let me maybe do a Note instead, that's probably better

[aidenfoxivey]

[aidenfoxivey]

@firewave any thoughts?

[firewave]

Posting test instead of images would be much better. That would also allow for it to be indexed and it would show up if you search for things.

@firewave any thoughts?

The note does not seem to improve things at all and just duplicates the existing message. I would have expected an actual fix-it hint.

[firewave]

I would have expected something like this:

<source>:1:45: warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
    1 | static constexpr unsigned int MYSTACKSIZE = 16*1024+32768;
      |                                             ^~~~~~~~
      |                                             (      )

https://godbolt.org/z/aWv1WzscG

That is for a clang-tidy check so I am not sure if a regular warning can provide the same.

[aidenfoxivey]

Ah okay, let me give that a go.

[aidenfoxivey]

@firewave I'm unclear though how it just duplicates the original message? It specifies to use a cast, no? I'll get a warning that shows to wrap the expression in a float() or a static_cast().

[firewave]

As this is about implicit conversions it is clear that you need an explicit cast to fix this. So that additional information is rather pointless.

The issue is that it is not clear which expression to actually cast. That clang-tidy example does that by showing the additional fix-it hint with the parentheses.