This is an archive of the discontinued LLVM Phabricator instance.

[libc] add printf decimal float conversion
ClosedPublic

Authored by michaelrj on Aug 2 2022, 2:36 PM.

Details

Summary

This patch adds support for converting doubles to string in the %f/F
format specifier. It does not yet support long doubles outside of the
double range. This implementation is based on the work of Ulf Adams,
specifically the Ryu Printf algorithm.

See:
Ulf Adams. 2019. Ryū revisited: printf floating point conversion.
Proc. ACM Program. Lang. 3, OOPSLA, Article 169 (October 2019), 23 pages.
https://doi.org/10.1145/3360595

Diff Detail

Event Timeline

michaelrj created this revision.Aug 2 2022, 2:36 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 2 2022, 2:36 PM
michaelrj requested review of this revision.Aug 2 2022, 2:36 PM

Just a bunch of nits for now. I will make an another pass once you fix them.

libc/src/stdio/printf_core/float_dec_converter.h
2

Decimal ?

39

While its good that we are citing the source of the algorithm, we should also try to explain the high level idea in prose (as in, not code) here.

44

You should use a const & argument here and elsewhere where functions take UInt<192> args as inputs.

46

Can you add comments explaining these constants and/or the algorithm here?

58

Specifics of which program?

66

There a mix of function naming convention in this file. Also, add a comment explaining what these functions are.

78

Use more modern casting style uint32_t(e). Elsewhere in this file as well.

83

Unsigned to signed conversion requires a comment.

91

uint64_t(1)

94

Feel free to add a constructor to UInt and take this out.

116

Why is this required?

118

Use member initializer list to initialize members.

158

Is this comment at the right place?

184

Looks like you are just making a copy of the PaddingWriter? In which case, const &?

194

Should this be private?

239

Can we not have write_first_block have a return value?

244

I think it guaranteed to not overflow block_buffer so may be just say it here in a comment?

365

Do you want it to be failure return always? AFAICT, it is being used below.

435

Add a comment explaining what this nonzero signifies because there is spread out code which conditions on this.

443

Use a symbolic constant for this, likely from FloatProperties or somewhere.

465

Will init be called multiple times in the outer for loop? If not, it feels like we can improve the flow here.

481

Is it OK to ignore the return value?

516

What is temporary?

569

Seems like write_last_block is not called from all paths? write_last_block does nothing now so it doesn't matter may be. But, the logical view feels incomplete now.

michaelrj updated this revision to Diff 454625.Aug 22 2022, 2:57 PM
michaelrj marked 21 inline comments as done.

address comments, clean up code, and add implementations (currently unused) for calculating the values in the tables.

libc/src/stdio/printf_core/float_dec_converter.h
58

adjusted phrasing.

116

it throws an error if there is no PaddingWriter default constructor since FloatWriter has it as a member.

365

write_last_block_exp will be used for the %e conversion, but is currently unused. write_last_block_dec is the one being used below, and is implemented above.

465

It will not. I've added a comment to explain a bit, but this loop is trying to find the highest non-zero digit before the decimal point. If it finds that digit, then we know exactly how long the number as a whole will be, since we know how many digits are remaining before the decimal point and the number of digits after the decimal point is just the precision. Those are the values the FloatWriter is being initialized with.

516

oops, forgot to clean out some comments.

569

as discussed above, there are two write_last_block functions and this one is actually complete. As for if it needs to be called on all paths, not quite. write_last_block_dec handles flushing the buffer, converting the last block into a string, and rounding. In the cases where we know that all of the final digits are zero, we can skip those steps and just call write_zeroes, which also flushes the buffer, and no rounding can be necessary since the trailing digits are all zero.

michaelrj updated this revision to Diff 456452.Aug 29 2022, 1:40 PM

add basic long double support (only for numbers with positive exponents and its very slow right now).

Few high level comments about organization but logic LGTM.

  1. The ryu_constants.h should be moved to a .cpp file.
  2. There are two aspects wrt float to string conversion - one is the printf specific code, for example padding and spaces. Other is the actual conversion algorithm. I think we should separate out the actual conversion algorithm into a separately utility of it own and use it to implement the printf converter. If I am reading this patch correctly, the two aspects are being mixed up or co-located. I happy to be convinced that it is not possible, but I want learn the reasons why if that is so.
libc/src/stdio/printf_core/float_dec_converter.h
2

Why should this live entirely in a header file?

split float conversion into a separate file

sivachandra added inline comments.Sep 19 2022, 10:38 AM
libc/src/__support/float_to_string.h
20

Two suggestions:

  1. Move the pieces internal to the implementation into an __internal namespace.
  2. Add a comment about how client code is to use this utility.
libc/src/stdio/printf_core/float_dec_converter.h
2

Is this answered somewhere?

33

Do these comment blocks belong here or next to the new utility?

michaelrj updated this revision to Diff 461375.Sep 19 2022, 2:44 PM
michaelrj marked 5 inline comments as done.

clean up and add comments.
Add internal namespace to contain many of the utilities.

libc/src/stdio/printf_core/float_dec_converter.h
2

Right now it's mostly to enable the use of the converter_atlas to replace the implementations of these conversion functions, although there isn't a particular reason beyond that.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 7 2022, 11:25 AM
This revision was automatically updated to reflect the committed changes.
libc/src/__support/float_to_string.h