Re: Ryu floating point output patch

Hi,

On 2018-12-13 19:41:55 +0000, Andrew Gierth wrote:
> This is a mostly cleaned-up version of the test patch I posted
> previously for floating-point output using the Ryu algorithm.

Nice!

> From the upstream, I've taken only specific files which are
> Boost-licensed, removed code not of interest to us, removed C99-isms,
> applied project style for things like type names and use of INT64CONST,
> removed some ad-hoc configuration #ifs in favour of using values
> established by pg_config.h, and ran the whole thing through pgindent and
> fixed up the resulting wreckage.

Removed C99isms? Why, we allow that these days? Did you mean C11?

> +static inline uint64
> +mulShiftAll(const uint64 m, const uint64 *const mul, const int32 j,
> + uint64 *const vp, uint64 *const vm, const uint32 mmShift)
> +{
> +/* m <<= 2; */
> +/* uint128 b0 = ((uint128) m) * mul[0]; // 0 */
> +/* uint128 b2 = ((uint128) m) * mul[1]; // 64 */
> +/* */
> +/* uint128 hi = (b0 >> 64) + b2; */
> +/* uint128 lo = b0 & 0xffffffffffffffffull; */
> +/* uint128 factor = (((uint128) mul[1]) << 64) + mul[0]; */
> +/* uint128 vpLo = lo + (factor << 1); */
> +/* *vp = (uint64) ((hi + (vpLo >> 64)) >> (j - 64)); */
> +/* uint128 vmLo = lo - (factor << mmShift); */
> +/* *vm = (uint64) ((hi + (vmLo >> 64) - (((uint128) 1ull) << 64)) >> (j - 64)); */
> +/* return (uint64) (hi >> (j - 64)); */
> + *vp = mulShift(4 * m + 2, mul, j);
> + *vm = mulShift(4 * m - 1 - mmShift, mul, j);
> + return mulShift(4 * m, mul, j);
> +}

What's with all the commented out code? I'm not sure that keeping close
alignment with the original codebase with things like that has much
value given the extent of the reformatting and functional changes?

> +/* These tables are generated by PrintDoubleLookupTable. */

This kind of reference is essentially dangling now, so perhaps we should
rewrite that?

> +++ b/src/common/d2s_intrinsics.h

> +static inline uint64
> +umul128(const uint64 a, const uint64 b, uint64 *const productHi)
> +{
> + return _umul128(a, b, productHi);
> +}

These are fairly generic names, perhaps we ought to prefix them?

Greetings,

Andres Freund