This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 8/9] Negative numbers added for sreal class.
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Martin LiÅka <mliska at suse dot cz>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 9 Dec 2014 15:15:34 +0100
- Subject: Re: [PATCH 8/9] Negative numbers added for sreal class.
- Authentication-results: sourceware.org; auth=none
- References: <398814b6afe28679f16c5d4b9879accb7984b76b dot 1415911038 dot git dot mliska at suse dot cz> <d26dfed99e27a9501f2fc5b76b93d4d2430c5efc dot 1415911038 dot git dot mliska at suse dot cz> <CAFiYyc1AsbETOUBY2ieE3ibU+v+8ypH33YBOfYBKJMNKzYZpQg at mail dot gmail dot com> <546F202F dot 8090606 at suse dot cz> <CAFiYyc2Nw8pr45hak_mHmhikWHk_84iZCYKtpyVyvJ_PX=0cJA at mail dot gmail dot com> <546F4EB7 dot 8060006 at suse dot cz> <CAFiYyc1=VQe86mdUab-tZN89z-XBHH3HNV1vsebELs-qJP=ZeQ at mail dot gmail dot com> <546F5877 dot 2050300 at suse dot cz> <54775A83 dot 5050107 at suse dot cz> <CAFiYyc160ZQiWNh2TQCBx6uKD3WFTyWUbtiLmCf2-RAiqkL+6w at mail dot gmail dot com> <5485D76B dot 6080902 at suse dot cz>
On Mon, Dec 8, 2014 at 5:52 PM, Martin LiÅka <mliska@suse.cz> wrote:
> 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.
That will ICE if you do
sreal x (- __LONG_MAX__ - 1);
maybe that's the only case though.
sreal::normalize ()
{
+ int64_t s = m_sig < 0 ? -1 : 1;
+ HOST_WIDE_INT sig = abs_hwi (m_sig);
+
if (m_sig == 0)
...
}
+
+ m_sig = s * sig;
}
it's a bit awkward to strip the sign and then put it back on this way. Also
now using a signed 'sig' where it was unsigned before. And keeping
the first test using m_sig instead of sig.
I'd simply have used 'unsigned HOST_WIDE_INT sig = absu_hwi (m_sig);'
instead.
The rest of the patch is ok with the above change.
Thanks,
Richard.
>>
>> + 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
>>>
>>>
>