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

Unexpected output from intel fmt::fprint #3645

Closed
gsjaardema opened this issue Sep 18, 2023 · 15 comments
Closed

Unexpected output from intel fmt::fprint #3645

gsjaardema opened this issue Sep 18, 2023 · 15 comments

Comments

@gsjaardema
Copy link
Contributor

gsjaardema commented Sep 18, 2023

With the following program:

#include <fmt/printf.h>
int main()
{
    fmt::printf("%d:%d %f %s\n", 3, 1234, 12.345, "weekend");
}

I get the expected output on most compiler and lib::fmt version combinations:

3:1234 12.345000 weekend

However, if I use the intel compiler with either 10.0.0 or trunk, I get:

3:1234 12.345001 weekend

Notice the 1 at the end of the floating point output. I'm not sure where this is coming from or how to get the same output on intel with 10.0.X as I get with intel and earlier lib::fmt versions or with all other compilers I have tried.

Here is a compiler explorer link: https://godbolt.org/z/MzG7T9bfr

@gsjaardema
Copy link
Contributor Author

I added the c-library printf call:

    printf("%d:%d %f %s\n", 3, 1234, 12.345, "weekend");

And that gives me the "expected" output:

3:1234 12.345000 weekend

@gsjaardema
Copy link
Contributor Author

gsjaardema commented Sep 18, 2023

I also tried with the %g format specifier for the double parameter and get:

3:1234 12.3451 weekend

which is worse accuracy.

If I loop from 0.345 to 99.345:

include <fmt/printf.h>
int main()
{
   for (int i = 0; i < 100; i++) { 
        double di = (double)i;
        fmt::printf("%d:%d %g %s\n", 3, 1234, di+.345, "weekend");
    }
}

The extraneous 1 is added on for these values for %g output:

3:1234 7.345 weekend
3:1234 8.34501 weekend
3:1234 9.34501 weekend
3:1234 10.3451 weekend
3:1234 11.3451 weekend
3:1234 12.3451 weekend
3:1234 13.3451 weekend
3:1234 14.3451 weekend
3:1234 15.3451 weekend
3:1234 16.345 weekend

And these for %f output:

3:1234 7.345000 weekend
3:1234 8.345001 weekend
3:1234 9.345001 weekend
3:1234 10.345001 weekend
3:1234 11.345001 weekend
3:1234 12.345001 weekend
3:1234 13.345001 weekend
3:1234 14.345001 weekend
3:1234 15.345001 weekend
3:1234 16.345000 weekend

@vitaut
Copy link
Contributor

vitaut commented Sep 18, 2023

Interestingly, the issue doesn't happen with print: https://godbolt.org/z/1s3ebbv9K.

@gsjaardema
Copy link
Contributor Author

Interestingly, the issue doesn't happen with print: https://godbolt.org/z/1s3ebbv9K.

Yes, I noticed that. The assembly output is also much cleaner and smaller.

@gsjaardema
Copy link
Contributor Author

We found another strangeness:

int main()
{
    fmt::printf("%g %g\n", -10., 2.);
}

Returns:

-10.0001 2.00001

@vitaut
Copy link
Contributor

vitaut commented Sep 18, 2023

Could be related to #3269.

@gsjaardema
Copy link
Contributor Author

Could be related to #3269.

That is where a git bisect points for me also.

@gsjaardema
Copy link
Contributor Author

Interestingly, the issue doesn't happen with print: https://godbolt.org/z/1s3ebbv9K.

Although it does it you define FMT_HEADER_ONLY. https://godbolt.org/z/vazbMba97

@vitaut
Copy link
Contributor

vitaut commented Sep 19, 2023

Maybe @jk-jeon has some insights regarding what might be causing this.

@jk-jeon
Copy link
Contributor

jk-jeon commented Sep 19, 2023

This is because the rounding criterion is evaluated incorrectly. The cause is that the static data member fmt::detail::data::fractional_part_rounding_thresholds is initialized with zeros for some reason I am not aware of. Fixing that will fix this issue I believe.

By the way, compiler explorer is really god send. Who can expect I can debug without even having a compiler.

@jk-jeon
Copy link
Contributor

jk-jeon commented Sep 19, 2023

It really sounds like a compiler bug: https://godbolt.org/z/9MKsa8xjq.
Apparently it only happens when the containing struct is a template and the member is an array.

@gsjaardema
Copy link
Contributor Author

So does basic_data need to be a template struct? I did remove the template <typename T = void> from struct basic_data and verified that it did fix the bug, but I don't know enough about the internals to know why it is a template structure...

@vitaut
Copy link
Contributor

vitaut commented Sep 19, 2023

So does basic_data need to be a template struct?

Yes, for the header-only mode. However, we are moving away from it in favor of storing such tables as static constexpr variables in functions which can be applied as a workaround here.

In any case, this is a severe bug in the Intel compiler, please report it.

@vitaut
Copy link
Contributor

vitaut commented Sep 19, 2023

Thanks, @jk-jeon, for investigating!

@phprus
Copy link
Contributor

phprus commented Sep 20, 2023

It really sounds like a compiler bug: https://godbolt.org/z/9MKsa8xjq.
Apparently it only happens when the containing struct is a template and the member is an array.

This is EDG frontend and C++ >= 17 bug.
More info: #3043

gsjaardema added a commit to gsjaardema/fmt that referenced this issue Sep 20, 2023
Potential workaround / restructure for the intel bug that is the cause of fmtlib#3645.

Make the variable in the external struct instead an embedded static constexpr variable in the only function that uses the variable.
vitaut pushed a commit that referenced this issue Sep 21, 2023
* Workaround intel bug

Potential workaround / restructure for the intel bug that is the cause of #3645.

Make the variable in the external struct instead an embedded static constexpr variable in the only function that uses the variable.

* Finish the proposed change -- remove struct accessor

* Refactor proposed intel fix.

Moved variable out of function to avoid specialization on Float.  Made it a separate function that is called from format_float.

* Fix incorrect function name.

* Add missing inline.
@vitaut vitaut closed this as completed Sep 21, 2023
ckerr pushed a commit to transmission/fmt that referenced this issue Nov 7, 2023
* Workaround intel bug

Potential workaround / restructure for the intel bug that is the cause of fmtlib#3645.

Make the variable in the external struct instead an embedded static constexpr variable in the only function that uses the variable.

* Finish the proposed change -- remove struct accessor

* Refactor proposed intel fix.

Moved variable out of function to avoid specialization on Float.  Made it a separate function that is called from format_float.

* Fix incorrect function name.

* Add missing inline.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants