[Qemu-devel] [RFC PATCH v2 0/2] softfloat: Add round-to-odd rounding mode

[Qemu-devel] [RFC PATCH v2 0/2] softfloat: Add round-to-odd rounding mode

[Bharata B Rao]

Hi,

Here is the next version of round-to-odd rounding mode implementation.

In this version I have addressed the reveiw comments from v1 and added
a new patch to take care of 64 bit rounding in underflow case. This
fix was found necessary when comparing the result of PowerPC ISA 3.0
instruction xscvqpdp between QEMU implementation and a known-good
implementation.

I have tested these patches and compared the results of
xsaddqp[o], xsmulqp[o], xsdivqp[o] and xscvqpdp[0] between QEMU
implementation and a known-good implementation.

I wanted to test with RISU to check if any older floating point
instructions (prior to PowerPC ISA v3.0) were affected by these
rounding changes. But even without my patchset, I am seeing RISU
reporting failures between QEMU implementation and P8 hardware.
While I am investigating the cause for these failures, I also plan
to do RISU verification for ISA 3.0 instructions with a known good
implementation.

Changes in v2:
-------------
- Do odd or even for the right precision bit in 64bit rounding. (Peter Maydell)
- Handle the overflow case correctly in 64bit rounding. (Peter Maydell)
- Add a patch to handle underflow case correctly in 64bit rounding.
 
v1: http://patchwork.ozlabs.org/patch/717562/

Bharata B Rao (2):
  softfloat: Handle float64 rounding properly for underflow case
  softfloat: Add round-to-odd rounding mode

 fpu/softfloat.c         | 34 +++++++++++++++++++++++++++++++++-
 include/fpu/softfloat.h |  2 ++
 2 files changed, 35 insertions(+), 1 deletion(-)

-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 1/2] softfloat: Handle float64 rounding properly for underflow case

[Bharata B Rao]

When rounding a floating point result to float64 precision, the
existing code doesn't re-calculate the required round increment
for the underflow case. Fix this.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 fpu/softfloat.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index c295f31..b04699c 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -651,6 +651,23 @@ static float64 roundAndPackFloat64(flag zSign, int zExp, uint64_t zSig,
             if (isTiny && roundBits) {
                 float_raise(float_flag_underflow, status);
             }
+            switch (roundingMode) {
+            case float_round_nearest_even:
+            case float_round_ties_away:
+                roundIncrement = 0x200;
+                break;
+            case float_round_to_zero:
+                roundIncrement = 0;
+                break;
+            case float_round_up:
+                roundIncrement = zSign ? 0 : 0x3ff;
+                break;
+            case float_round_down:
+                roundIncrement = zSign ? 0x3ff : 0;
+                break;
+            default:
+                abort();
+            }
         }
     }
     if (roundBits) {
-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 2/2] softfloat: Add round-to-odd rounding mode

[Bharata B Rao]

Power ISA 3.0 introduces a few quadruple precision floating point
instructions that support round-to-odd rounding mode. The
round-to-odd mode is explained as under:

Let Z be the intermediate arithmetic result or the operand of a convert
operation. If Z can be represented exactly in the target format, the
result is Z. Otherwise the result is either Z1 or Z2 whichever is odd.
Here Z1 and Z2 are the next larger and smaller numbers representable
in the target format respectively.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 fpu/softfloat.c         | 17 ++++++++++++++++-
 include/fpu/softfloat.h |  2 ++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index b04699c..1c322ad 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -623,6 +623,9 @@ static float64 roundAndPackFloat64(flag zSign, int zExp, uint64_t zSig,
     case float_round_down:
         roundIncrement = zSign ? 0x3ff : 0;
         break;
+    case float_round_to_odd:
+        roundIncrement = (zSig & 0x400) ? 0 : 0x3ff;
+        break;
     default:
         abort();
     }
@@ -632,8 +635,10 @@ static float64 roundAndPackFloat64(flag zSign, int zExp, uint64_t zSig,
              || (    ( zExp == 0x7FD )
                   && ( (int64_t) ( zSig + roundIncrement ) < 0 ) )
            ) {
+            bool overflow_to_inf = roundingMode != float_round_to_odd &&
+                                   roundIncrement != 0;
             float_raise(float_flag_overflow | float_flag_inexact, status);
-            return packFloat64( zSign, 0x7FF, - ( roundIncrement == 0 ));
+            return packFloat64( zSign, 0x7FF, - ( !overflow_to_inf ));
         }
         if ( zExp < 0 ) {
             if (status->flush_to_zero) {
@@ -665,6 +670,9 @@ static float64 roundAndPackFloat64(flag zSign, int zExp, uint64_t zSig,
             case float_round_down:
                 roundIncrement = zSign ? 0x3ff : 0;
                 break;
+            case float_round_to_odd:
+                roundIncrement = (zSig & 0x400) ? 0 : 0x3ff;
+                break;
             default:
                 abort();
             }
@@ -1166,6 +1174,9 @@ static float128 roundAndPackFloat128(flag zSign, int32_t zExp,
     case float_round_down:
         increment = zSign && zSig2;
         break;
+    case float_round_to_odd:
+        increment = !(zSig1 & 0x1) && zSig2;
+        break;
     default:
         abort();
     }
@@ -1185,6 +1196,7 @@ static float128 roundAndPackFloat128(flag zSign, int32_t zExp,
             if (    ( roundingMode == float_round_to_zero )
                  || ( zSign && ( roundingMode == float_round_up ) )
                  || ( ! zSign && ( roundingMode == float_round_down ) )
+                 || ( roundingMode == float_round_to_odd )
                ) {
                 return
                     packFloat128(
@@ -1232,6 +1244,9 @@ static float128 roundAndPackFloat128(flag zSign, int32_t zExp,
             case float_round_down:
                 increment = zSign && zSig2;
                 break;
+            case float_round_to_odd:
+                increment = !(zSig1 & 0x1) && zSig2;
+                break;
             default:
                 abort();
             }
diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index 842ec6b..8a39028 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -180,6 +180,8 @@ enum {
     float_round_up           = 2,
     float_round_to_zero      = 3,
     float_round_ties_away    = 4,
+    /* Not an IEEE rounding mode: round to the closest odd mantissa value */
+    float_round_to_odd       = 5,
 };
 
 /*----------------------------------------------------------------------------
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH v2 0/2] softfloat: Add round-to-odd rounding mode

[no-reply]

Hi,

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

Type: series
Subject: [Qemu-devel] [RFC PATCH v2 0/2] softfloat: Add round-to-odd rounding mode
Message-id: 1485504213-21632-1-git-send-email-bharata@linux.vnet.ibm.com

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

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

# Useful git options
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
 * [new tag]         patchew/1485504213-21632-1-git-send-email-bharata@linux.vnet.ibm.com -> patchew/1485504213-21632-1-git-send-email-bharata@linux.vnet.ibm.com
Switched to a new branch 'test'
3adbab9 softfloat: Add round-to-odd rounding mode
afe7f83 softfloat: Handle float64 rounding properly for underflow case

=== OUTPUT BEGIN ===
Checking PATCH 1/2: softfloat: Handle float64 rounding properly for underflow case...
Checking PATCH 2/2: softfloat: Add round-to-odd rounding mode...
ERROR: space prohibited after that '-' (ctx:WxW)
#41: FILE: fpu/softfloat.c:641:
+            return packFloat64( zSign, 0x7FF, - ( !overflow_to_inf ));
                                               ^

ERROR: space prohibited after that open parenthesis '('
#41: FILE: fpu/softfloat.c:641:
+            return packFloat64( zSign, 0x7FF, - ( !overflow_to_inf ));

ERROR: space prohibited before that close parenthesis ')'
#41: FILE: fpu/softfloat.c:641:
+            return packFloat64( zSign, 0x7FF, - ( !overflow_to_inf ));

ERROR: space prohibited after that open parenthesis '('
#69: FILE: fpu/softfloat.c:1199:
+                 || ( roundingMode == float_round_to_odd )

ERROR: space prohibited before that close parenthesis ')'
#69: FILE: fpu/softfloat.c:1199:
+                 || ( roundingMode == float_round_to_odd )

total: 5 errors, 0 warnings, 62 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

Re: [Qemu-devel] [RFC PATCH v2 0/2] softfloat: Add round-to-odd rounding mode

[Bharata B Rao]

On Fri, Jan 27, 2017 at 12:09:13AM -0800, no-reply@patchew.org wrote:
> Hi,
> 
> Your series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Subject: [Qemu-devel] [RFC PATCH v2 0/2] softfloat: Add round-to-odd rounding mode
> Message-id: 1485504213-21632-1-git-send-email-bharata@linux.vnet.ibm.com
> 
> Checking PATCH 1/2: softfloat: Handle float64 rounding properly for underflow case...
> Checking PATCH 2/2: softfloat: Add round-to-odd rounding mode...
> ERROR: space prohibited after that '-' (ctx:WxW)
> #41: FILE: fpu/softfloat.c:641:
> +            return packFloat64( zSign, 0x7FF, - ( !overflow_to_inf ));
>                                                ^
> 
> ERROR: space prohibited after that open parenthesis '('
> #41: FILE: fpu/softfloat.c:641:
> +            return packFloat64( zSign, 0x7FF, - ( !overflow_to_inf ));
> 
> ERROR: space prohibited before that close parenthesis ')'
> #41: FILE: fpu/softfloat.c:641:
> +            return packFloat64( zSign, 0x7FF, - ( !overflow_to_inf ));
> 
> ERROR: space prohibited after that open parenthesis '('
> #69: FILE: fpu/softfloat.c:1199:
> +                 || ( roundingMode == float_round_to_odd )
> 
> ERROR: space prohibited before that close parenthesis ')'
> #69: FILE: fpu/softfloat.c:1199:
> +                 || ( roundingMode == float_round_to_odd )
> 
> total: 5 errors, 0 warnings, 62 lines checked

fpu/softfloat.c follows a different coding style and hence it made
sense to stick to the existing style in the file.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v2 0/2] softfloat: Add round-to-odd rounding mode

[Bharata B Rao]

Peter,

On Fri, Jan 27, 2017 at 01:33:31PM +0530, Bharata B Rao wrote:
> 
> Changes in v2:
> -------------
> - Do odd or even for the right precision bit in 64bit rounding. (Peter Maydell)
> - Handle the overflow case correctly in 64bit rounding. (Peter Maydell)

Does this version of round-to-odd implementation look correct now ?

> - Add a patch to handle underflow case correctly in 64bit rounding.

Does this fix for underflow case make sense ?

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v2 1/2] softfloat: Handle float64 rounding properly for underflow case

[Peter Maydell]

On 27 January 2017 at 08:03, Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> When rounding a floating point result to float64 precision, the
> existing code doesn't re-calculate the required round increment
> for the underflow case. Fix this.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  fpu/softfloat.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index c295f31..b04699c 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -651,6 +651,23 @@ static float64 roundAndPackFloat64(flag zSign, int zExp, uint64_t zSig,
>              if (isTiny && roundBits) {
>                  float_raise(float_flag_underflow, status);
>              }
> +            switch (roundingMode) {
> +            case float_round_nearest_even:
> +            case float_round_ties_away:
> +                roundIncrement = 0x200;
> +                break;
> +            case float_round_to_zero:
> +                roundIncrement = 0;
> +                break;
> +            case float_round_up:
> +                roundIncrement = zSign ? 0 : 0x3ff;
> +                break;
> +            case float_round_down:
> +                roundIncrement = zSign ? 0x3ff : 0;
> +                break;
> +            default:
> +                abort();
> +            }
>          }
>      }
>      if (roundBits) {

When does this give a different value to what roundIncrement
was before? As far as I can see the only data-dependency
here is on zSign, and underflowing doesn't affect zSign.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v2 2/2] softfloat: Add round-to-odd rounding mode

[Peter Maydell]

On 27 January 2017 at 08:03, Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> Power ISA 3.0 introduces a few quadruple precision floating point
> instructions that support round-to-odd rounding mode. The
> round-to-odd mode is explained as under:
>
> Let Z be the intermediate arithmetic result or the operand of a convert
> operation. If Z can be represented exactly in the target format, the
> result is Z. Otherwise the result is either Z1 or Z2 whichever is odd.
> Here Z1 and Z2 are the next larger and smaller numbers representable
> in the target format respectively.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  fpu/softfloat.c         | 17 ++++++++++++++++-
>  include/fpu/softfloat.h |  2 ++
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index b04699c..1c322ad 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -623,6 +623,9 @@ static float64 roundAndPackFloat64(flag zSign, int zExp, uint64_t zSig,
>      case float_round_down:
>          roundIncrement = zSign ? 0x3ff : 0;
>          break;
> +    case float_round_to_odd:
> +        roundIncrement = (zSig & 0x400) ? 0 : 0x3ff;
> +        break;
>      default:
>          abort();
>      }
> @@ -632,8 +635,10 @@ static float64 roundAndPackFloat64(flag zSign, int zExp, uint64_t zSig,
>               || (    ( zExp == 0x7FD )
>                    && ( (int64_t) ( zSig + roundIncrement ) < 0 ) )
>             ) {
> +            bool overflow_to_inf = roundingMode != float_round_to_odd &&
> +                                   roundIncrement != 0;
>              float_raise(float_flag_overflow | float_flag_inexact, status);
> -            return packFloat64( zSign, 0x7FF, - ( roundIncrement == 0 ));
> +            return packFloat64( zSign, 0x7FF, - ( !overflow_to_inf ));

Can you follow the QEMU coding style, please? softfloat's coding
style is insane and we generally have been switching it to the
QEMU style as we touch lines of code.

>          }
>          if ( zExp < 0 ) {
>              if (status->flush_to_zero) {
> @@ -665,6 +670,9 @@ static float64 roundAndPackFloat64(flag zSign, int zExp, uint64_t zSig,
>              case float_round_down:
>                  roundIncrement = zSign ? 0x3ff : 0;
>                  break;
> +            case float_round_to_odd:
> +                roundIncrement = (zSig & 0x400) ? 0 : 0x3ff;
> +                break;
>              default:
>                  abort();
>              }

Ah, this extra check does make a difference for the round-to-odd
case. But since it's a special for round-to-odd, better to
phrase as
  if (roundingMode == float_round_to_odd) {
      /* in this case roundIncrement depends on zSig, which just changed */
      roundIncrement = ...;
  }

rather than a case statement where most of the code is no-ops.

> @@ -1166,6 +1174,9 @@ static float128 roundAndPackFloat128(flag zSign, int32_t zExp,
>      case float_round_down:
>          increment = zSign && zSig2;
>          break;
> +    case float_round_to_odd:
> +        increment = !(zSig1 & 0x1) && zSig2;
> +        break;
>      default:
>          abort();
>      }
> @@ -1185,6 +1196,7 @@ static float128 roundAndPackFloat128(flag zSign, int32_t zExp,
>              if (    ( roundingMode == float_round_to_zero )
>                   || ( zSign && ( roundingMode == float_round_up ) )
>                   || ( ! zSign && ( roundingMode == float_round_down ) )
> +                 || ( roundingMode == float_round_to_odd )
>                 ) {
>                  return
>                      packFloat128(
> @@ -1232,6 +1244,9 @@ static float128 roundAndPackFloat128(flag zSign, int32_t zExp,
>              case float_round_down:
>                  increment = zSign && zSig2;
>                  break;
> +            case float_round_to_odd:
> +                increment = !(zSig1 & 0x1) && zSig2;
> +                break;
>              default:
>                  abort();
>              }
> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
> index 842ec6b..8a39028 100644
> --- a/include/fpu/softfloat.h
> +++ b/include/fpu/softfloat.h
> @@ -180,6 +180,8 @@ enum {
>      float_round_up           = 2,
>      float_round_to_zero      = 3,
>      float_round_ties_away    = 4,
> +    /* Not an IEEE rounding mode: round to the closest odd mantissa value */
> +    float_round_to_odd       = 5,
>  };
>
>  /*----------------------------------------------------------------------------
> --
> 2.7.4
>

Otherwise:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM