Re: [Qemu-devel] [PULL 18/22] fpu/softfloat: re-factor int/uint to float

2018-05-08 Thread Alex Bennée 

Peter Maydell  writes:

> On 27 April 2018 at 14:49, Alex Bennée  wrote:
>>
>> Peter Maydell  writes:
>>
>>> On 21 February 2018 at 11:05, Alex Bennée  wrote:
 +/*
 + * Integer to float conversions
 + *
 + * Returns the result of converting the two's complement integer `a'
 + * to the floating-point format. The conversion is performed according
 + * to the IEC/IEEE Standard for Binary Floating-Point Arithmetic.
 + */
 +
 +static FloatParts int_to_float(int64_t a, float_status *status)
 +{
 +FloatParts r;
 +if (a == 0) {
 +r.cls = float_class_zero;
 +r.sign = false;
 +} else if (a == (1ULL << 63)) {
 +r.cls = float_class_normal;
 +r.sign = true;
 +r.frac = DECOMPOSED_IMPLICIT_BIT;
 +r.exp = 63;
 +} else {
 +uint64_t f;
 +if (a < 0) {
 +f = -a;
 +r.sign = true;
 +} else {
 +f = a;
 +r.sign = false;
 +}
 +int shift = clz64(f) - 1;
 +r.cls = float_class_normal;
 +r.exp = (DECOMPOSED_BINARY_POINT - shift);
 +r.frac = f << shift;
 +}
 +
 +return r;
 +}
>>>
>>> Hi -- Coverity complains about this function (CID1390635) because
>>> there's a code path through it that doesn't fully initialize
>>> the struct (the a == 0 case doesn't set r.frac), and it thinks
>>> that "return r" is a 'use' of all fields in the structure.
>>> I don't know why it doesn't complain about r.exp.
>>>
>>> Should we initialize all of r's fields anyway to shut it up,
>>> or mark it as a Coverity false-positive?
>>
>> Hmm tricky - because in some cases we don't want to mess with it. The
>> fcvt patch for example manually messed with the frac portion to deal
>> with conversion. But it's done outside of the main canonicalize/pack
>> routines.
>
> Hmm? The problem is only in this specific function, which
> currently returns an entirely uninitialized 'frac' field,
> and could be made to return a zeroed field. There aren't
> many other functions that create a FloatParts struct themselves
> rather than modifying an existing one:
>  * unpack_raw() sets all the fields
>  * uint_to_float() has "FloatParts r = { .sign = false};"
>which implicitly sets all the other parts to 0
>
> It doesn't seem too unreasonable to me to either have this
> function start "FloatParts r = {};" or to init both exp
> and frac in the "zero" case.

Good point - it is the source of the FloatParts in this case. I'll init
it.

>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PULL 18/22] fpu/softfloat: re-factor int/uint to float

2018-05-08 Thread Peter Maydell 
On 27 April 2018 at 14:49, Alex Bennée  wrote:
>
> Peter Maydell  writes:
>
>> On 21 February 2018 at 11:05, Alex Bennée  wrote:
>>> +/*
>>> + * Integer to float conversions
>>> + *
>>> + * Returns the result of converting the two's complement integer `a'
>>> + * to the floating-point format. The conversion is performed according
>>> + * to the IEC/IEEE Standard for Binary Floating-Point Arithmetic.
>>> + */
>>> +
>>> +static FloatParts int_to_float(int64_t a, float_status *status)
>>> +{
>>> +FloatParts r;
>>> +if (a == 0) {
>>> +r.cls = float_class_zero;
>>> +r.sign = false;
>>> +} else if (a == (1ULL << 63)) {
>>> +r.cls = float_class_normal;
>>> +r.sign = true;
>>> +r.frac = DECOMPOSED_IMPLICIT_BIT;
>>> +r.exp = 63;
>>> +} else {
>>> +uint64_t f;
>>> +if (a < 0) {
>>> +f = -a;
>>> +r.sign = true;
>>> +} else {
>>> +f = a;
>>> +r.sign = false;
>>> +}
>>> +int shift = clz64(f) - 1;
>>> +r.cls = float_class_normal;
>>> +r.exp = (DECOMPOSED_BINARY_POINT - shift);
>>> +r.frac = f << shift;
>>> +}
>>> +
>>> +return r;
>>> +}
>>
>> Hi -- Coverity complains about this function (CID1390635) because
>> there's a code path through it that doesn't fully initialize
>> the struct (the a == 0 case doesn't set r.frac), and it thinks
>> that "return r" is a 'use' of all fields in the structure.
>> I don't know why it doesn't complain about r.exp.
>>
>> Should we initialize all of r's fields anyway to shut it up,
>> or mark it as a Coverity false-positive?
>
> Hmm tricky - because in some cases we don't want to mess with it. The
> fcvt patch for example manually messed with the frac portion to deal
> with conversion. But it's done outside of the main canonicalize/pack
> routines.

Hmm? The problem is only in this specific function, which
currently returns an entirely uninitialized 'frac' field,
and could be made to return a zeroed field. There aren't
many other functions that create a FloatParts struct themselves
rather than modifying an existing one:
 * unpack_raw() sets all the fields
 * uint_to_float() has "FloatParts r = { .sign = false};"
   which implicitly sets all the other parts to 0

It doesn't seem too unreasonable to me to either have this
function start "FloatParts r = {};" or to init both exp
and frac in the "zero" case.

thanks
-- PMM

Re: [Qemu-devel] [PULL 18/22] fpu/softfloat: re-factor int/uint to float

2018-04-27 Thread Alex Bennée 

Peter Maydell  writes:

> On 21 February 2018 at 11:05, Alex Bennée  wrote:
>> +/*
>> + * Integer to float conversions
>> + *
>> + * Returns the result of converting the two's complement integer `a'
>> + * to the floating-point format. The conversion is performed according
>> + * to the IEC/IEEE Standard for Binary Floating-Point Arithmetic.
>> + */
>> +
>> +static FloatParts int_to_float(int64_t a, float_status *status)
>> +{
>> +FloatParts r;
>> +if (a == 0) {
>> +r.cls = float_class_zero;
>> +r.sign = false;
>> +} else if (a == (1ULL << 63)) {
>> +r.cls = float_class_normal;
>> +r.sign = true;
>> +r.frac = DECOMPOSED_IMPLICIT_BIT;
>> +r.exp = 63;
>> +} else {
>> +uint64_t f;
>> +if (a < 0) {
>> +f = -a;
>> +r.sign = true;
>> +} else {
>> +f = a;
>> +r.sign = false;
>> +}
>> +int shift = clz64(f) - 1;
>> +r.cls = float_class_normal;
>> +r.exp = (DECOMPOSED_BINARY_POINT - shift);
>> +r.frac = f << shift;
>> +}
>> +
>> +return r;
>> +}
>
> Hi -- Coverity complains about this function (CID1390635) because
> there's a code path through it that doesn't fully initialize
> the struct (the a == 0 case doesn't set r.frac), and it thinks
> that "return r" is a 'use' of all fields in the structure.
> I don't know why it doesn't complain about r.exp.
>
> Should we initialize all of r's fields anyway to shut it up,
> or mark it as a Coverity false-positive?

Hmm tricky - because in some cases we don't want to mess with it. The
fcvt patch for example manually messed with the frac portion to deal
with conversion. But it's done outside of the main canonicalize/pack
routines.

>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PULL 18/22] fpu/softfloat: re-factor int/uint to float

2018-04-27 Thread Peter Maydell 
On 21 February 2018 at 11:05, Alex Bennée  wrote:
> +/*
> + * Integer to float conversions
> + *
> + * Returns the result of converting the two's complement integer `a'
> + * to the floating-point format. The conversion is performed according
> + * to the IEC/IEEE Standard for Binary Floating-Point Arithmetic.
> + */
> +
> +static FloatParts int_to_float(int64_t a, float_status *status)
> +{
> +FloatParts r;
> +if (a == 0) {
> +r.cls = float_class_zero;
> +r.sign = false;
> +} else if (a == (1ULL << 63)) {
> +r.cls = float_class_normal;
> +r.sign = true;
> +r.frac = DECOMPOSED_IMPLICIT_BIT;
> +r.exp = 63;
> +} else {
> +uint64_t f;
> +if (a < 0) {
> +f = -a;
> +r.sign = true;
> +} else {
> +f = a;
> +r.sign = false;
> +}
> +int shift = clz64(f) - 1;
> +r.cls = float_class_normal;
> +r.exp = (DECOMPOSED_BINARY_POINT - shift);
> +r.frac = f << shift;
> +}
> +
> +return r;
> +}

Hi -- Coverity complains about this function (CID1390635) because
there's a code path through it that doesn't fully initialize
the struct (the a == 0 case doesn't set r.frac), and it thinks
that "return r" is a 'use' of all fields in the structure.
I don't know why it doesn't complain about r.exp.

Should we initialize all of r's fields anyway to shut it up,
or mark it as a Coverity false-positive?

thanks
-- PMM

[Qemu-devel] [PULL 18/22] fpu/softfloat: re-factor int/uint to float

2018-02-21 Thread Alex Bennée 
These are considerably simpler as the lower order integers can just
use the higher order conversion function. As the decomposed fractional
part is a full 64 bit rounding and inexact handling comes from the
pack functions.

Signed-off-by: Alex Bennée 
Reviewed-by: Richard Henderson 

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index da0c43c0e7..4313d3a602 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -1500,6 +1500,169 @@ FLOAT_TO_UINT(64, 64)
 
 #undef FLOAT_TO_UINT
 
+/*
+ * Integer to float conversions
+ *
+ * Returns the result of converting the two's complement integer `a'
+ * to the floating-point format. The conversion is performed according
+ * to the IEC/IEEE Standard for Binary Floating-Point Arithmetic.
+ */
+
+static FloatParts int_to_float(int64_t a, float_status *status)
+{
+FloatParts r;
+if (a == 0) {
+r.cls = float_class_zero;
+r.sign = false;
+} else if (a == (1ULL << 63)) {
+r.cls = float_class_normal;
+r.sign = true;
+r.frac = DECOMPOSED_IMPLICIT_BIT;
+r.exp = 63;
+} else {
+uint64_t f;
+if (a < 0) {
+f = -a;
+r.sign = true;
+} else {
+f = a;
+r.sign = false;
+}
+int shift = clz64(f) - 1;
+r.cls = float_class_normal;
+r.exp = (DECOMPOSED_BINARY_POINT - shift);
+r.frac = f << shift;
+}
+
+return r;
+}
+
+float16 int64_to_float16(int64_t a, float_status *status)
+{
+FloatParts pa = int_to_float(a, status);
+return float16_round_pack_canonical(pa, status);
+}
+
+float16 int32_to_float16(int32_t a, float_status *status)
+{
+return int64_to_float16(a, status);
+}
+
+float16 int16_to_float16(int16_t a, float_status *status)
+{
+return int64_to_float16(a, status);
+}
+
+float32 int64_to_float32(int64_t a, float_status *status)
+{
+FloatParts pa = int_to_float(a, status);
+return float32_round_pack_canonical(pa, status);
+}
+
+float32 int32_to_float32(int32_t a, float_status *status)
+{
+return int64_to_float32(a, status);
+}
+
+float32 int16_to_float32(int16_t a, float_status *status)
+{
+return int64_to_float32(a, status);
+}
+
+float64 int64_to_float64(int64_t a, float_status *status)
+{
+FloatParts pa = int_to_float(a, status);
+return float64_round_pack_canonical(pa, status);
+}
+
+float64 int32_to_float64(int32_t a, float_status *status)
+{
+return int64_to_float64(a, status);
+}
+
+float64 int16_to_float64(int16_t a, float_status *status)
+{
+return int64_to_float64(a, status);
+}
+
+
+/*
+ * Unsigned Integer to float conversions
+ *
+ * Returns the result of converting the unsigned integer `a' to the
+ * floating-point format. The conversion is performed according to the
+ * IEC/IEEE Standard for Binary Floating-Point Arithmetic.
+ */
+
+static FloatParts uint_to_float(uint64_t a, float_status *status)
+{
+FloatParts r = { .sign = false};
+
+if (a == 0) {
+r.cls = float_class_zero;
+} else {
+int spare_bits = clz64(a) - 1;
+r.cls = float_class_normal;
+r.exp = DECOMPOSED_BINARY_POINT - spare_bits;
+if (spare_bits < 0) {
+shift64RightJamming(a, -spare_bits, );
+r.frac = a;
+} else {
+r.frac = a << spare_bits;
+}
+}
+
+return r;
+}
+
+float16 uint64_to_float16(uint64_t a, float_status *status)
+{
+FloatParts pa = uint_to_float(a, status);
+return float16_round_pack_canonical(pa, status);
+}
+
+float16 uint32_to_float16(uint32_t a, float_status *status)
+{
+return uint64_to_float16(a, status);
+}
+
+float16 uint16_to_float16(uint16_t a, float_status *status)
+{
+return uint64_to_float16(a, status);
+}
+
+float32 uint64_to_float32(uint64_t a, float_status *status)
+{
+FloatParts pa = uint_to_float(a, status);
+return float32_round_pack_canonical(pa, status);
+}
+
+float32 uint32_to_float32(uint32_t a, float_status *status)
+{
+return uint64_to_float32(a, status);
+}
+
+float32 uint16_to_float32(uint16_t a, float_status *status)
+{
+return uint64_to_float32(a, status);
+}
+
+float64 uint64_to_float64(uint64_t a, float_status *status)
+{
+FloatParts pa = uint_to_float(a, status);
+return float64_round_pack_canonical(pa, status);
+}
+
+float64 uint32_to_float64(uint32_t a, float_status *status)
+{
+return uint64_to_float64(a, status);
+}
+
+float64 uint16_to_float64(uint16_t a, float_status *status)
+{
+return uint64_to_float64(a, status);
+}
+
 /*
 | Takes a 64-bit fixed-point value `absZ' with binary point between bits 6
 | and 7, and returns the properly rounded 32-bit integer corresponding to the
@@ -2591,43 +2754,6 @@ static float128 normalizeRoundAndPackFloat128(flag 
zSign, int32_t zExp,
 
 }