This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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
>>>
>>>
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]