On Thu, Nov 27, 2014 at 6:08 PM, Martin Liška <mli...@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 <mli...@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
+ 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?
That is, you effectively use the 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 ...
Richard.
> I am able to run profiled bootstrap on x86_64-linux-pc and ppc64-linux-pc
> and new regression is introduced.
>
> Thanks,
> Martin
>
>
