On Mon, Dec 8, 2014 at 5:52 PM, Martin Liška <mli...@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 <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 > > > 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 >>> >>> >