[libc-commits] [PATCH] D131023: [libc] add printf decimal float conversion
Siva Chandra via Phabricator via libc-commits
libc-commits at lists.llvm.org
Mon Aug 15 17:05:01 PDT 2022
sivachandra added a comment.
Just a bunch of nits for now. I will make an another pass once you fix them.
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:1
+//===-- Hexadecimal Converter for printf ------------------------*- C++ -*-===//
+//
----------------
`Decimal` ?
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:38
+// This version is modified to require significantly less memory (it doesn't use
+// a large buffer to store the result) in exchange for a slower runtime.
+
----------------
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.
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:43
+
+static inline uint32_t fast_uint_mod_1e9(cpp::UInt<192> val) {
+ const cpp::UInt<192> mult_const(
----------------
You should use a `const &` argument here and elsewhere where functions take `UInt<192>` args as inputs.
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:45
+ const cpp::UInt<192> mult_const(
+ {0x31680A88F8953031u, 0x89705F4136B4A597u, 0});
+ const auto middle = (mult_const * val);
----------------
Can you add comments explaining these constants and/or the algorithm here?
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:57
+ uint64_t carry = val.mul(small);
+ // due to the specifics of this program, we know that shift_amount will always
+ // be at least 128.
----------------
Specifics of which program?
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:65
+
+static inline uint32_t indexForExponent(const uint32_t e) {
+ return (e + 15) / 16;
----------------
There a mix of function naming convention in this file. Also, add a comment explaining what these functions are.
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:77
+ // greater than 10^297. assert(e >= 0); assert(e <= 1650);
+ return (((uint32_t)e) * 78913) >> 18;
+}
----------------
Use more modern casting style `uint32_t(e)`. Elsewhere in this file as well.
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:82
+ // +1 for ceil, +16 for mantissa, +8 to round up when dividing by 9
+ return (log10Pow2(16 * (int32_t)idx) + 1 + 16 + 8) / 9;
+}
----------------
Unsigned to signed conversion requires a comment.
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:90
+ // __builtin_ctzll doesn't appear to be faster here.
+ return (value & ((1ull << p) - 1)) == 0;
+}
----------------
`uint64_t(1)`
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:93
+
+static inline cpp::UInt<192> ptr_to_uint(const uint64_t nums[3]) {
+ cpp::UInt<192> result;
----------------
Feel free to add a constructor to `UInt` and take this out.
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:115
+public:
+ PaddingWriter() { ; }
+ PaddingWriter(const FormatSection &to_conv, char init_sign_char) {
----------------
Why is this required?
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:117
+ PaddingWriter(const FormatSection &to_conv, char init_sign_char) {
+ left_justified = (to_conv.flags & FormatFlags::LEFT_JUSTIFIED) > 0;
+ leading_zeroes = (to_conv.flags & FormatFlags::LEADING_ZEROES) > 0;
----------------
Use member initializer list to initialize members.
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:157
+/*
+ We only need to round a given segment if all of the segments below it are
+ the max (or this is the last segment). This means that we don't have to
----------------
Is this comment at the right place?
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:183
+ FloatWriter(Writer *init_writer, bool init_has_decimal_point,
+ PaddingWriter &init_padding_writer) {
+ writer = init_writer;
----------------
Looks like you are just making a copy of the `PaddingWriter`? In which case, `const &`?
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:193
+
+ int flush_buffer() {
+ // Write the most recent buffered block, and mark has_written
----------------
Should this be private?
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:238
+
+ int write_first_block(uint32_t block) {
+ auto const int_to_str = integer_to_string(block);
----------------
Can we not have `write_first_block` have a return value?
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:243
+ for (size_t count = 0; count < digits_written; ++count) {
+ block_buffer[count] = int_to_str.str()[count];
+ }
----------------
I think it guaranteed to not overflow `block_buffer` so may be just say it here in a comment?
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:364
+ // Also we need to write the exponent, but that's pretty simple.
+ return -1;
+ }
----------------
Do you want it to be failure return always? AFAICT, it is being used below.
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:434
+
+ bool nonzero = false;
+
----------------
Add a comment explaining what this `nonzero` signifies because there is spread out code which conditions on this.
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:442
+
+ if (exponent >= -52) {
+ const uint32_t idx =
----------------
Use a symbolic constant for this, likely from `FloatProperties` or somewhere.
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:464
+ size_t blocks_before_decimal = i;
+ float_writer.init((blocks_before_decimal * BLOCK_SIZE) +
+ (has_decimal_point ? 1 : 0) + precision,
----------------
Will `init` be called multiple times in the outer for loop? If not, it feels like we can improve the flow here.
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:480
+ // POW10_SPLIT[POW10_OFFSET[idx] + i] = table[e][i]
+ float_writer.write_middle_block(digits);
+ }
----------------
Is it OK to ignore the return value?
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:515
+ }
+ // Temporary: j is usually around 128, and by shifting a bit, we push it
+ // to 128 or above, which is a slightly faster code path in
----------------
What is temporary?
================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:568
+ RET_IF_RESULT_NEGATIVE(
+ float_writer.write_last_block_dec(digits, maximum, round));
+ break;
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131023/new/
https://reviews.llvm.org/D131023
More information about the libc-commits
mailing list