Hi,

In my previous run at this I'd simply taken the existing float32
functions and attempted to copy and paste the code changing the
relevant constants. Apart from the usual typos and missed bits there
were sections where softfloat pulls tricks because it knows the exact
bit positions of things. While I'm sure it's marginally faster it does
make the code rather impenetrable to someone not familiar with how
SoftFloat does things. One thing the last few months have taught me is
the world is not awash with experts on the finer implementation
details of floating point maths. After reviewing the last series
Richard Henderson suggested a different approach which pushed most of
the code into common shared functions. The majority of the work on the
fractional bits is done in 64 bit resolution which leaves plenty of
spare bits for rounding for everything from float16 to float64. This
series is a result of that work and a coding sprint we did 2 weeks ago
in Cambridge.

We've not touched anything that needs higher precision which at the
moment is float80 and 128 bit quad precision operations. They would
need similar decomposed routines to operate on the higher precision
fractional parts. I suspect we'd need to beef up our Int128 wrapper in
the process so it can be done efficiently with 128 bit maths.

This work is part of the larger chunk of adding half-precision ops to
the ARM front-end. However I've split the series up to make for a less
messy review. This tree can be found at:

  https://github.com/stsquad/qemu/tree/softfloat-refactor-and-fp16-v1

While I have been testing the half-precision stuff in the ARM
specific tree this series is all common code. It has however been
tested with ARM RISU which exercises the float32/64 code paths quite
nicely.

Any additional testing appreciated.

Series Breakdown
----------------

The first five patches are simple helper functions that are mostly
inline and there for the benefit of architecture helper functions.
This includes the float16 constants in the final patch.

The next two patches fixed a bug in NaN propagation which only showed
up when doing ARM "Reduction" operations in float16. Although the
minmax code is totally replaced later on I wanted to fix it in place
first rather than add the fix when it was re-written.

The next two patches start preparing the ground for the new decomposed
functions and their public APIs. I've used macro expansion in a few
places just to avoid the amount of repeated boiler-plate for these
APIs. Most of the work is done in the static decompose_foo functions.

As you can see in the diffstat there is an overall code reduction even
though we have also added float16 support. For reference the previous
attempt added 1258 lines of code to implement a subset of the float16
functions. I think the code is also a lot easier to follow and reason
about.

Alex Bennée (19):
  fpu/softfloat: implement float16_squash_input_denormal
  include/fpu/softfloat: implement float16_abs helper
  include/fpu/softfloat: implement float16_chs helper
  include/fpu/softfloat: implement float16_set_sign helper
  include/fpu/softfloat: add some float16 contants
  fpu/softfloat: propagate signalling NaNs in MINMAX
  fpu/softfloat: improve comments on ARM NaN propagation
  fpu/softfloat: move the extract functions to the top of the file
  fpu/softfloat: define decompose structures
  fpu/softfloat: re-factor add/sub
  fpu/softfloat: re-factor mul
  fpu/softfloat: re-factor div
  fpu/softfloat: re-factor muladd
  fpu/softfloat: re-factor round_to_int
  fpu/softfloat: re-factor float to int/uint
  fpu/softfloat: re-factor int/uint to float
  fpu/softfloat: re-factor scalbn
  fpu/softfloat: re-factor minmax
  fpu/softfloat: re-factor compare

 fpu/softfloat-macros.h     |   44 +
 fpu/softfloat-specialize.h |  115 +-
 fpu/softfloat.c            | 6668 ++++++++++++++++++++------------------------
 include/fpu/softfloat.h    |   89 +-
 4 files changed, 3066 insertions(+), 3850 deletions(-)

-- 
2.15.1

Hi,

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

Message-id: 20171211125705.16120-1-alex.bennee@linaro.org
Subject: [Qemu-devel] [PATCH v1 00/19] re-factor softfloat and add fp16 functions
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20171205171706.23947-1-ybettan@redhat.com -> patchew/20171205171706.23947-1-ybettan@redhat.com
Switched to a new branch 'test'
45c20d7e14 fpu/softfloat: re-factor compare
b52eaabb36 fpu/softfloat: re-factor minmax
518bbcee3e fpu/softfloat: re-factor scalbn
208ff4370f fpu/softfloat: re-factor int/uint to float
85ef39bd87 fpu/softfloat: re-factor float to int/uint
7ae34923a2 fpu/softfloat: re-factor round_to_int
c8662d53c5 fpu/softfloat: re-factor muladd
c95c5f4c5f fpu/softfloat: re-factor div
d8ac851b38 fpu/softfloat: re-factor mul
174b42de84 fpu/softfloat: re-factor add/sub
c93e9f8a58 fpu/softfloat: define decompose structures
31f8322049 fpu/softfloat: move the extract functions to the top of the file
2a8a9e64a0 fpu/softfloat: improve comments on ARM NaN propagation
d1eefad57b fpu/softfloat: propagate signalling NaNs in MINMAX
e052270783 include/fpu/softfloat: add some float16 contants
149ba6ff6d include/fpu/softfloat: implement float16_set_sign helper
9921bf4d5a include/fpu/softfloat: implement float16_chs helper
0665d6d615 include/fpu/softfloat: implement float16_abs helper
36fce4ec4a fpu/softfloat: implement float16_squash_input_denormal

=== OUTPUT BEGIN ===
Checking PATCH 1/19: fpu/softfloat: implement float16_squash_input_denormal...
Checking PATCH 2/19: include/fpu/softfloat: implement float16_abs helper...
Checking PATCH 3/19: include/fpu/softfloat: implement float16_chs helper...
Checking PATCH 4/19: include/fpu/softfloat: implement float16_set_sign helper...
Checking PATCH 5/19: include/fpu/softfloat: add some float16 contants...
Checking PATCH 6/19: fpu/softfloat: propagate signalling NaNs in MINMAX...
Checking PATCH 7/19: fpu/softfloat: improve comments on ARM NaN propagation...
Checking PATCH 8/19: fpu/softfloat: move the extract functions to the top of the file...
Checking PATCH 9/19: fpu/softfloat: define decompose structures...
ERROR: spaces prohibited around that ':' (ctx:WxW)
#54: FILE: fpu/softfloat.c:210:
+    uint64_t frac   : 64;
                     ^

ERROR: spaces prohibited around that ':' (ctx:WxW)
#55: FILE: fpu/softfloat.c:211:
+    int exp         : 32;
                     ^

ERROR: space prohibited before that ':' (ctx:WxW)
#57: FILE: fpu/softfloat.c:213:
+    int             : 23;
                     ^

ERROR: spaces prohibited around that ':' (ctx:WxW)
#58: FILE: fpu/softfloat.c:214:
+    bool sign       : 1;
                     ^

total: 4 errors, 0 warnings, 84 lines checked

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

Checking PATCH 10/19: fpu/softfloat: re-factor add/sub...
WARNING: line over 80 characters
#140: FILE: fpu/softfloat.c:364:
+                                                   const decomposed_params *parm)

total: 0 errors, 1 warnings, 937 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 11/19: fpu/softfloat: re-factor mul...
Checking PATCH 12/19: fpu/softfloat: re-factor div...
Checking PATCH 13/19: fpu/softfloat: re-factor muladd...
Checking PATCH 14/19: fpu/softfloat: re-factor round_to_int...
WARNING: line over 80 characters
#90: FILE: fpu/softfloat.c:1252:
+                inc = ((a.frac & roundeven_mask) != frac_lsbm1 ? frac_lsbm1 : 0);

total: 0 errors, 1 warnings, 329 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 15/19: fpu/softfloat: re-factor float to int/uint...
ERROR: space prohibited after that '-' (ctx:WxW)
#55: FILE: fpu/softfloat.c:1347:
+            return r < - (uint64_t) INT64_MIN ? -r : INT64_MIN;
                        ^

WARNING: line over 80 characters
#91: FILE: fpu/softfloat.c:1383:
+int ## isz ## _t float ## fsz ## _to_int ## isz(float ## fsz a, float_status *s) \

WARNING: line over 80 characters
#171: FILE: fpu/softfloat.c:1463:
+uint ## isz ## _t float ## fsz ## _to_uint ## isz(float ## fsz a, float_status *s) \

ERROR: space prohibited after that open parenthesis '('
#711: FILE: fpu/softfloat.c:3410:
+    if (    ( ( extractFloat32Exp( a ) == 0xFF ) && extractFloat32Frac( a ) )

ERROR: space prohibited before that close parenthesis ')'
#711: FILE: fpu/softfloat.c:3410:
+    if (    ( ( extractFloat32Exp( a ) == 0xFF ) && extractFloat32Frac( a ) )

ERROR: space prohibited after that open parenthesis '('
#712: FILE: fpu/softfloat.c:3411:
+         || ( ( extractFloat32Exp( b ) == 0xFF ) && extractFloat32Frac( b ) )

ERROR: space prohibited before that close parenthesis ')'
#712: FILE: fpu/softfloat.c:3411:
+         || ( ( extractFloat32Exp( b ) == 0xFF ) && extractFloat32Frac( b ) )

ERROR: space prohibited after that open parenthesis '('
#733: FILE: fpu/softfloat.c:3419:
+    aSign = extractFloat32Sign( a );

ERROR: space prohibited before that close parenthesis ')'
#733: FILE: fpu/softfloat.c:3419:
+    aSign = extractFloat32Sign( a );

ERROR: space prohibited after that open parenthesis '('
#734: FILE: fpu/softfloat.c:3420:
+    bSign = extractFloat32Sign( b );

ERROR: space prohibited before that close parenthesis ')'
#734: FILE: fpu/softfloat.c:3420:
+    bSign = extractFloat32Sign( b );

WARNING: line over 80 characters
#737: FILE: fpu/softfloat.c:3423:
+    if ( aSign != bSign ) return aSign && ( (uint32_t) ( ( av | bv )<<1 ) != 0 );

ERROR: spaces required around that '<<' (ctx:VxV)
#737: FILE: fpu/softfloat.c:3423:
+    if ( aSign != bSign ) return aSign && ( (uint32_t) ( ( av | bv )<<1 ) != 0 );
                                                                     ^

ERROR: space prohibited after that open parenthesis '('
#737: FILE: fpu/softfloat.c:3423:
+    if ( aSign != bSign ) return aSign && ( (uint32_t) ( ( av | bv )<<1 ) != 0 );

ERROR: space prohibited before that close parenthesis ')'
#737: FILE: fpu/softfloat.c:3423:
+    if ( aSign != bSign ) return aSign && ( (uint32_t) ( ( av | bv )<<1 ) != 0 );

ERROR: trailing statements should be on next line
#737: FILE: fpu/softfloat.c:3423:
+    if ( aSign != bSign ) return aSign && ( (uint32_t) ( ( av | bv )<<1 ) != 0 );

ERROR: braces {} are necessary for all arms of this statement
#737: FILE: fpu/softfloat.c:3423:
+    if ( aSign != bSign ) return aSign && ( (uint32_t) ( ( av | bv )<<1 ) != 0 );
[...]

ERROR: space prohibited after that open parenthesis '('
#738: FILE: fpu/softfloat.c:3424:
+    return ( av != bv ) && ( aSign ^ ( av < bv ) );

ERROR: space prohibited before that close parenthesis ')'
#738: FILE: fpu/softfloat.c:3424:
+    return ( av != bv ) && ( aSign ^ ( av < bv ) );

ERROR: space prohibited after that open parenthesis '('
#798: FILE: fpu/softfloat.c:3440:
+    if (    ( ( extractFloat32Exp( a ) == 0xFF ) && extractFloat32Frac( a ) )

ERROR: space prohibited before that close parenthesis ')'
#798: FILE: fpu/softfloat.c:3440:
+    if (    ( ( extractFloat32Exp( a ) == 0xFF ) && extractFloat32Frac( a ) )

ERROR: space prohibited after that open parenthesis '('
#799: FILE: fpu/softfloat.c:3441:
+         || ( ( extractFloat32Exp( b ) == 0xFF ) && extractFloat32Frac( b ) )

ERROR: space prohibited before that close parenthesis ')'
#799: FILE: fpu/softfloat.c:3441:
+         || ( ( extractFloat32Exp( b ) == 0xFF ) && extractFloat32Frac( b ) )

total: 20 errors, 3 warnings, 1065 lines checked

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

Checking PATCH 16/19: fpu/softfloat: re-factor int/uint to float...
Checking PATCH 17/19: fpu/softfloat: re-factor scalbn...
Checking PATCH 18/19: fpu/softfloat: re-factor minmax...
WARNING: line over 80 characters
#122: FILE: fpu/softfloat.c:1764:
+float ## sz float ## sz ## _ ## name(float ## sz a, float ## sz b, float_status *s) \

total: 0 errors, 1 warnings, 266 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 19/19: fpu/softfloat: re-factor compare...
WARNING: line over 80 characters
#88: FILE: fpu/softfloat.c:1864:
+int float ## sz ## _compare_quiet(float ## sz a, float ## sz b, float_status *s) \

total: 0 errors, 1 warnings, 155 lines checked

Your patch 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


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

no-reply@patchew.org writes:

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

FWIW these are either:

  - misidentified "spaces prohibited around that ':' (ctx:WxW)" for bitfields
  - existing softfloat code that has moved
  - two lines that just edge over the 80 char limit

> Checking PATCH 9/19: fpu/softfloat: define decompose structures...
> ERROR: spaces prohibited around that ':' (ctx:WxW)
> #54: FILE: fpu/softfloat.c:210:
> +    uint64_t frac   : 64;
>                      ^
>
> ERROR: spaces prohibited around that ':' (ctx:WxW)
> #55: FILE: fpu/softfloat.c:211:
> +    int exp         : 32;
>                      ^
>
> ERROR: space prohibited before that ':' (ctx:WxW)
> #57: FILE: fpu/softfloat.c:213:
> +    int             : 23;
>                      ^
>
> ERROR: spaces prohibited around that ':' (ctx:WxW)
> #58: FILE: fpu/softfloat.c:214:
> +    bool sign       : 1;
>                      ^
>
> total: 4 errors, 0 warnings, 84 lines checked
>
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> Checking PATCH 10/19: fpu/softfloat: re-factor add/sub...
> WARNING: line over 80 characters
> #140: FILE: fpu/softfloat.c:364:
> +                                                   const decomposed_params *parm)
>
> total: 0 errors, 1 warnings, 937 lines checked
>
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> Checking PATCH 11/19: fpu/softfloat: re-factor mul...
> Checking PATCH 12/19: fpu/softfloat: re-factor div...
> Checking PATCH 13/19: fpu/softfloat: re-factor muladd...
> Checking PATCH 14/19: fpu/softfloat: re-factor round_to_int...
> WARNING: line over 80 characters
> #90: FILE: fpu/softfloat.c:1252:
> +                inc = ((a.frac & roundeven_mask) != frac_lsbm1 ? frac_lsbm1 : 0);
>
> total: 0 errors, 1 warnings, 329 lines checked
>
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> Checking PATCH 15/19: fpu/softfloat: re-factor float to int/uint...
> ERROR: space prohibited after that '-' (ctx:WxW)
> #55: FILE: fpu/softfloat.c:1347:
> +            return r < - (uint64_t) INT64_MIN ? -r : INT64_MIN;
>                         ^
>
> WARNING: line over 80 characters
> #91: FILE: fpu/softfloat.c:1383:
> +int ## isz ## _t float ## fsz ## _to_int ## isz(float ## fsz a, float_status *s) \
>
> WARNING: line over 80 characters
> #171: FILE: fpu/softfloat.c:1463:
> +uint ## isz ## _t float ## fsz ## _to_uint ## isz(float ## fsz a, float_status *s) \
>
> ERROR: space prohibited after that open parenthesis '('
> #711: FILE: fpu/softfloat.c:3410:
> +    if (    ( ( extractFloat32Exp( a ) == 0xFF ) && extractFloat32Frac( a ) )
>
> ERROR: space prohibited before that close parenthesis ')'
> #711: FILE: fpu/softfloat.c:3410:
> +    if (    ( ( extractFloat32Exp( a ) == 0xFF ) && extractFloat32Frac( a ) )
>
> ERROR: space prohibited after that open parenthesis '('
> #712: FILE: fpu/softfloat.c:3411:
> +         || ( ( extractFloat32Exp( b ) == 0xFF ) && extractFloat32Frac( b ) )
>
> ERROR: space prohibited before that close parenthesis ')'
> #712: FILE: fpu/softfloat.c:3411:
> +         || ( ( extractFloat32Exp( b ) == 0xFF ) && extractFloat32Frac( b ) )
>
> ERROR: space prohibited after that open parenthesis '('
> #733: FILE: fpu/softfloat.c:3419:
> +    aSign = extractFloat32Sign( a );
>
> ERROR: space prohibited before that close parenthesis ')'
> #733: FILE: fpu/softfloat.c:3419:
> +    aSign = extractFloat32Sign( a );
>
> ERROR: space prohibited after that open parenthesis '('
> #734: FILE: fpu/softfloat.c:3420:
> +    bSign = extractFloat32Sign( b );
>
> ERROR: space prohibited before that close parenthesis ')'
> #734: FILE: fpu/softfloat.c:3420:
> +    bSign = extractFloat32Sign( b );
>
> WARNING: line over 80 characters
> #737: FILE: fpu/softfloat.c:3423:
> +    if ( aSign != bSign ) return aSign && ( (uint32_t) ( ( av | bv )<<1 ) != 0 );
>
> ERROR: spaces required around that '<<' (ctx:VxV)
> #737: FILE: fpu/softfloat.c:3423:
> +    if ( aSign != bSign ) return aSign && ( (uint32_t) ( ( av | bv )<<1 ) != 0 );
>                                                                      ^
>
> ERROR: space prohibited after that open parenthesis '('
> #737: FILE: fpu/softfloat.c:3423:
> +    if ( aSign != bSign ) return aSign && ( (uint32_t) ( ( av | bv )<<1 ) != 0 );
>
> ERROR: space prohibited before that close parenthesis ')'
> #737: FILE: fpu/softfloat.c:3423:
> +    if ( aSign != bSign ) return aSign && ( (uint32_t) ( ( av | bv )<<1 ) != 0 );
>
> ERROR: trailing statements should be on next line
> #737: FILE: fpu/softfloat.c:3423:
> +    if ( aSign != bSign ) return aSign && ( (uint32_t) ( ( av | bv )<<1 ) != 0 );
>
> ERROR: braces {} are necessary for all arms of this statement
> #737: FILE: fpu/softfloat.c:3423:
> +    if ( aSign != bSign ) return aSign && ( (uint32_t) ( ( av | bv )<<1 ) != 0 );
> [...]
>
> ERROR: space prohibited after that open parenthesis '('
> #738: FILE: fpu/softfloat.c:3424:
> +    return ( av != bv ) && ( aSign ^ ( av < bv ) );
>
> ERROR: space prohibited before that close parenthesis ')'
> #738: FILE: fpu/softfloat.c:3424:
> +    return ( av != bv ) && ( aSign ^ ( av < bv ) );
>
> ERROR: space prohibited after that open parenthesis '('
> #798: FILE: fpu/softfloat.c:3440:
> +    if (    ( ( extractFloat32Exp( a ) == 0xFF ) && extractFloat32Frac( a ) )
>
> ERROR: space prohibited before that close parenthesis ')'
> #798: FILE: fpu/softfloat.c:3440:
> +    if (    ( ( extractFloat32Exp( a ) == 0xFF ) && extractFloat32Frac( a ) )
>
> ERROR: space prohibited after that open parenthesis '('
> #799: FILE: fpu/softfloat.c:3441:
> +         || ( ( extractFloat32Exp( b ) == 0xFF ) && extractFloat32Frac( b ) )
>
> ERROR: space prohibited before that close parenthesis ')'
> #799: FILE: fpu/softfloat.c:3441:
> +         || ( ( extractFloat32Exp( b ) == 0xFF ) && extractFloat32Frac( b ) )
>
> total: 20 errors, 3 warnings, 1065 lines checked
>
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> Checking PATCH 16/19: fpu/softfloat: re-factor int/uint to float...
> Checking PATCH 17/19: fpu/softfloat: re-factor scalbn...
> Checking PATCH 18/19: fpu/softfloat: re-factor minmax...
> WARNING: line over 80 characters
> #122: FILE: fpu/softfloat.c:1764:
> +float ## sz float ## sz ## _ ## name(float ## sz a, float ## sz b, float_status *s) \
>
> total: 0 errors, 1 warnings, 266 lines checked
>
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> Checking PATCH 19/19: fpu/softfloat: re-factor compare...
> WARNING: line over 80 characters
> #88: FILE: fpu/softfloat.c:1864:
> +int float ## sz ## _compare_quiet(float ## sz a, float ## sz b, float_status *s) \
>
> total: 0 errors, 1 warnings, 155 lines checked
<snip>

--
Alex Bennée