[PATCH 8/9] Negative numbers added for sreal class.

[Martin Liška]

On 11/28/2014 10:32 AM, Richard Biener wrote:
> On Thu, Nov 27, 2014 at 6:08 PM, Martin Liška <mliska@suse.cz> wrote:
>> On 11/21/2014 04:21 PM, Martin Liška wrote:
>>>
>>> On 11/21/2014 04:02 PM, Richard Biener wrote:
>>>>
>>>> On Fri, Nov 21, 2014 at 3:39 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>
>>>>> Hello.
>>>>>
>>>>> Ok, this is simplified, one can use sreal a = 12345 and it works ;)
>>>>>
>>>>>> that's a  new API, right?  There is no max () and I think that using
>>>>>> LONG_MIN here is asking for trouble (host dependence).  The
>>>>>> comment in the file says the max should be
>>>>>> sreal (SREAL_MAX_SIG, SREAL_MAX_EXP) and the min
>>>>>> sreal (-SREAL_MAX_SIG, SREAL_MAX_EXP)?
>>>>>>
>>>>>
>>>>> Sure, sreal can store much bigger(smaller) numbers :)
>>>>>
>>>>>> Where do you need sreal::to_double?  The host shouldn't perform
>>>>>> double calculations so it can be only for dumping?  In which case
>>>>>> the user should have used sreal::dump (), maybe with extra
>>>>>> arguments.
>>>>>>
>>>>>
>>>>> That new function was request from Honza, only for debugging purpose.
>>>>> I agree that dump should this kind of job.
>>>>>
>>>>> If no other problem, I will run tests once more and commit it.
>>>>> Thanks,
>>>>> Martin
>>>>
>>>>
>>>> -#define SREAL_MAX_EXP (INT_MAX / 4)
>>>> +#define SREAL_MAX_EXP (INT_MAX / 8)
>>>>
>>>> this change doesn't look necessary anymore?
>>>>
>>>> Btw, it's also odd that...
>>>>
>>>>    #define SREAL_PART_BITS 32
>>>> ...
>>>>    #define SREAL_MIN_SIG ((uint64_t) 1 << (SREAL_PART_BITS - 1))
>>>>    #define SREAL_MAX_SIG (((uint64_t) 1 << SREAL_PART_BITS) - 1)
>>>>
>>>> thus all m_sig values fit in 32bits but we still use a uint64_t m_sig ...
>>>> (the implementation uses 64bit for internal computations, but still
>>>> the storage is wasteful?)
>>>>
>>>> Of course the way normalize() works requires that storage to be
>>>> 64bits to store unnormalized values.
>>>>
>>>> I'd say ok with the SREAL_MAX_EXP change reverted.
>>>>
>>>
>>> Hi.
>>>
>>> You are right, this change was done because I used one bit for m_negative
>>> (bitfield), not needed any more.
>>>
>>> Final version attached.
>>>
>>> Thank you,
>>> Martin
>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>
>>>>>
>>>>>> Otherwise looks good to me and sorry for not noticing the above
>>>>>> earlier.
>>>>>>
>>>>>> Thanks,
>>>>>> Richard.
>>>>>>
>>>>>>> Thanks,
>>>>>>> Martin
>>>>>>>
>>>>>>>
>>>>>>>>>      };
>>>>>>>>>
>>>>>>>>>      extern void debug (sreal &ref);
>>>>>>>>> @@ -76,12 +133,12 @@ inline sreal &operator+= (sreal &a, const sreal
>>>>>>>>> &b)
>>>>>>>>>
>>>>>>>>>      inline sreal &operator-= (sreal &a, const sreal &b)
>>>>>>>>>      {
>>>>>>>>> -return a = a - b;
>>>>>>>>> +  return a = a - b;
>>>>>>>>>      }
>>>>>>>>>
>>>>>>>>>      inline sreal &operator/= (sreal &a, const sreal &b)
>>>>>>>>>      {
>>>>>>>>> -return a = a / b;
>>>>>>>>> +  return a = a / b;
>>>>>>>>>      }
>>>>>>>>>
>>>>>>>>>      inline sreal &operator*= (sreal &a, const sreal &b)
>>>>>>>>> --
>>>>>>>>> 2.1.2
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>
>> Hello.
>>
>> After IRC discussions, I decided to give sreal another refactoring where I
>> use int64_t for m_sig.
>>
>> This approach looks much easier and straightforward. I would like to
>> ask folk for comments?
>
> I think you want to exclude LONG_MIN (how do you know that LONG_MIN
> is min(int64_t)?) as a valid value for m_sig - after all SREAL_MIN_SIG
> makes it symmetric.  Or simply use abs_hwi at

Hello.

I decided to use abs_hwi.

>
> +  int64_t s = m_sig < 0 ? -1 : 1;
> +  uint64_t sig = m_sig == LONG_MIN ? LONG_MAX : std::abs (m_sig);
>
> -#define SREAL_MIN_SIG ((uint64_t) 1 << (SREAL_PART_BITS - 1))
> -#define SREAL_MAX_SIG (((uint64_t) 1 << SREAL_PART_BITS) - 1)
> +#define SREAL_MIN_SIG ((uint64_t) 1 << (SREAL_PART_BITS - 2))
> +#define SREAL_MAX_SIG (((uint64_t) 1 << (SREAL_PART_BITS - 1)) - 1)
>
> shouldn't this also be -2 in the last line?

It's correct, in the first line, I s/'SREAL_PART_BITS - 1'/'SREAL_PART_BITS - 2' and
second one is also decremented: s/'SREAL_PART_BITS'/'REAL_PART_BITS - 1'.

>
> That is, you effectively use the MSB as a sign bit?

Yes. It uses signed integer with MSB as a sign bit.

>
> Btw, a further change would be to make m_sig 'signed int', matching
> the "real" storage requirements according to SREAL_PART_BITS.
> This would of course still require temporaries used for computation
> to be 64bits and it would require normalization to work on the
> temporaries.  But then we'd get down to 8 bytes storage ...

Agree, we can shrink the size for future.

I've been running tests, I hope this implementation is much nicer than
having bool m_negative. What do you think about trunk acceptation?

Thanks,
Martin

>
> Richard.
>
>
>> I am able to run profiled bootstrap on x86_64-linux-pc and ppc64-linux-pc
>> and new regression is introduced.
>>
>> Thanks,
>> Martin
>>
>>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-New-sreal-implementation-which-uses-int64_t-as-m_sig.patch
Type: text/x-patch
Size: 10388 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20141208/945e6ef6/attachment.bin>