[RFC,0/8] fpu: experimental conversion of float128_addsub

Hi Richard,

This is the current state of my experiment to convert a 128 bit float
function (in this case addsub). The actual conversion was fairly
simple (basically a copy & paste with some tweaks for using the
unint128 inline functions). However I ran into a number of stumbles
with the int128.h support including casting of values like ~(Uint128)0
and messing around to handle things like missing __builtin support for
clz. I suspect having some of the #defines expand into uint128_*
functions plays some part in the 4x growth in code compared to the old
version. However the drop in performance is a lot less than that.

In terms of total code churn we replace each deleted line in
softfloat.c with 2 lines of new code although I suspect if we pressed
on we could reduce the diffstat deficit. Debugging the actual failures
was relatively painless with rr and the new code - perhaps because I
just find it easier to follow.

I've included your early patches as that happened to be the state of
my tree when I branched off. If we want to go forward with a more
complete conversion I guess we would need:

  - a more complete int128.h conversion (including fallback for non CONFIG_INT128)
  - seeing if some of the resulting code bloat can be reduced
  - seeing what scope there is for commonality of special case handling

I'm not a fan of having so much duplication but at least I personally
find the code is more readable.

Alex Bennée (3):
  int128.h: add bunch of uint128 utility functions (INCOMPLETE)
  tests/fp: add quad support to the benchmark utility
  softfloat: implement addsub_floats128 using Uint128 and new style code

Richard Henderson (5):
  softfloat: Use mulu64 for mul64To128
  softfloat: Use int128.h for some operations
  softfloat: Tidy a * b + inf return
  softfloat: Add float_cmask and constants
  softfloat: Inline pick_nan_muladd into its caller

 include/fpu/softfloat-macros.h |  80 ++--
 include/qemu/int128.h          | 122 ++++++
 fpu/softfloat.c                | 697 ++++++++++++++++++++-------------
 tests/fp/fp-bench.c            |  88 ++++-
 fpu/softfloat-specialize.c.inc |  39 ++
 5 files changed, 711 insertions(+), 315 deletions(-)

Patchew URL: https://patchew.org/QEMU/20201020163738.27700-1-alex.bennee@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20201020163738.27700-1-alex.bennee@linaro.org
Subject: [RFC PATCH  0/8] fpu: experimental conversion of float128_addsub

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20201020163738.27700-1-alex.bennee@linaro.org -> patchew/20201020163738.27700-1-alex.bennee@linaro.org
Switched to a new branch 'test'
165b2d1 softfloat: implement addsub_floats128 using Uint128 and new style code
cfb628a tests/fp: add quad support to the benchmark utility
593cf15 int128.h: add bunch of uint128 utility functions (INCOMPLETE)
83e37cd softfloat: Inline pick_nan_muladd into its caller
45f794e softfloat: Add float_cmask and constants
27be22d softfloat: Tidy a * b + inf return
e3c7e01 softfloat: Use int128.h for some operations
321df84 softfloat: Use mulu64 for mul64To128

=== OUTPUT BEGIN ===
1/8 Checking commit 321df844302f (softfloat: Use mulu64 for mul64To128)
2/8 Checking commit e3c7e01abc34 (softfloat: Use int128.h for some operations)
3/8 Checking commit 27be22d0ba4c (softfloat: Tidy a * b + inf return)
4/8 Checking commit 45f794e02b18 (softfloat: Add float_cmask and constants)
5/8 Checking commit 83e37cdf6e25 (softfloat: Inline pick_nan_muladd into its caller)
6/8 Checking commit 593cf1552545 (int128.h: add bunch of uint128 utility functions (INCOMPLETE))
7/8 Checking commit cfb628ae8542 (tests/fp: add quad support to the benchmark utility)
WARNING: line over 80 characters
#177: FILE: tests/fp/fp-bench.c:458:
+    GEN_BENCH_NO_NEG(bench_ ## name ## _float128, float128, PREC_FLOAT128, op, n)

WARNING: line over 80 characters
#194: FILE: tests/fp/fp-bench.c:521:
+    fprintf(stderr, " -p = floating point precision (single, double, quad[soft only]). "

total: 0 errors, 2 warnings, 185 lines checked

Patch 7/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
8/8 Checking commit 165b2d1b1292 (softfloat: implement addsub_floats128 using Uint128 and new style code)
WARNING: line over 80 characters
#101: FILE: fpu/softfloat.c:532:
+#define DECOMPOSED128_IMPLICIT_BIT (uint128_lshift(1, DECOMPOSED128_BINARY_POINT))

WARNING: line over 80 characters
#102: FILE: fpu/softfloat.c:533:
+#define DECOMPOSED128_OVERFLOW_BIT (uint128_lshift(DECOMPOSED128_IMPLICIT_BIT, 1))

WARNING: line over 80 characters
#200: FILE: fpu/softfloat.c:746:
+static FloatParts128 sf128_canonicalize(FloatParts128 part, const FloatFmt *parm,

WARNING: line over 80 characters
#229: FILE: fpu/softfloat.c:775:
+        part.frac = uint128_add(imp_bit, uint128_lshift(part.frac, parm->frac_shift));

ERROR: line over 90 characters
#246: FILE: fpu/softfloat.c:937:
+    const Uint128 frac_lsb = uint128_lshift(1, DECOMPOSED128_BINARY_POINT - parm->frac_size);

ERROR: line over 90 characters
#247: FILE: fpu/softfloat.c:938:
+    const Uint128 frac_lsbm1 = uint128_lshift(1, (DECOMPOSED128_BINARY_POINT - parm->frac_size) - 1);

ERROR: line over 90 characters
#249: FILE: fpu/softfloat.c:940:
+    const Uint128 roundeven_mask = uint128_sub(uint128_lshift(2, DECOMPOSED128_BINARY_POINT - parm->frac_size), 1);

ERROR: line over 90 characters
#319: FILE: fpu/softfloat.c:1010:
+                        || !(uint128_and(uint128_add(frac, inc), DECOMPOSED128_OVERFLOW_BIT));

WARNING: line over 80 characters
#404: FILE: fpu/softfloat.c:1263:
+static FloatParts128 pick_nan128(FloatParts128 a, FloatParts128 b, float_status *s)

WARNING: line over 80 characters
#425: FILE: fpu/softfloat.c:1284:
+static FloatParts128 addsub_floats128(FloatParts128 a, FloatParts128 b, bool subtract,

ERROR: space prohibited after that open parenthesis '('
#780: FILE: include/fpu/softfloat-macros.h:269:
+    } else if ( count < 128 ) {

ERROR: space prohibited before that close parenthesis ')'
#780: FILE: include/fpu/softfloat-macros.h:269:
+    } else if ( count < 128 ) {

ERROR: space prohibited after that open parenthesis '('
#785: FILE: include/fpu/softfloat-macros.h:274:
+        out = ( in != 0 );

ERROR: space prohibited before that close parenthesis ')'
#785: FILE: include/fpu/softfloat-macros.h:274:
+        out = ( in != 0 );

total: 8 errors, 6 warnings, 747 lines checked

Patch 8/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20201020163738.27700-1-alex.bennee@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com