On 12/09/2014 03:15 PM, Richard Biener wrote:
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.
Hello.
You are right that unsigned type would be more suitable.
I send final version that I plan to commit.
Thanks,
Martin
+ 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
>From b3a25b754e076c6a50b091ea783c2fd045a79647 Mon Sep 17 00:00:00 2001
From: mliska <mli...@suse.cz>
Date: Wed, 26 Nov 2014 15:46:42 +0100
Subject: [PATCH] New sreal implementation which uses int64_t as m_sig.
gcc/ChangeLog:
2014-12-08 Martin Liska <mli...@suse.cz>
* sreal.c (sreal::shift_right): New implementation
for int64_t as m_sig.
(sreal::normalize): Likewise.
(sreal::to_int): Likewise.
(sreal::operator+): Likewise.
(sreal::operator-): Likewise.
(sreal::operator*): Likewise.
(sreal::operator/): Likewise.
(sreal::signedless_minus): Removed.
(sreal::signedless_plus): Removed.
(sreal::debug): const keyword is added.
* sreal.h (sreal::operator<): New implementation
for int64_t as m_sig.
* ipa-inline.c (recursive_inlining): LONG_MIN is replaced
with sreal::min ().
---
gcc/ipa-inline.c | 2 +-
gcc/sreal.c | 131 +++++++++++++++++--------------------------------------
gcc/sreal.h | 56 ++++++++++--------------
3 files changed, 63 insertions(+), 126 deletions(-)
diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
index f62760f..9f600b0 100644
--- a/gcc/ipa-inline.c
+++ b/gcc/ipa-inline.c
@@ -1311,7 +1311,7 @@ recursive_inlining (struct cgraph_edge *edge,
vec<cgraph_edge *> *new_edges)
{
int limit = PARAM_VALUE (PARAM_MAX_INLINE_INSNS_RECURSIVE_AUTO);
- edge_heap_t heap (LONG_MIN);
+ edge_heap_t heap (sreal::min ());
struct cgraph_node *node;
struct cgraph_edge *e;
struct cgraph_node *master_clone = NULL, *next;
diff --git a/gcc/sreal.c b/gcc/sreal.c
index 2b5e3ae..bc3af23 100644
--- a/gcc/sreal.c
+++ b/gcc/sreal.c
@@ -61,13 +61,13 @@ sreal::dump (FILE *file) const
}
DEBUG_FUNCTION void
-debug (sreal &ref)
+debug (const sreal &ref)
{
ref.dump (stderr);
}
DEBUG_FUNCTION void
-debug (sreal *ptr)
+debug (const sreal *ptr)
{
if (ptr)
debug (*ptr);
@@ -91,7 +91,7 @@ sreal::shift_right (int s)
m_exp += s;
- m_sig += (uint64_t) 1 << (s - 1);
+ m_sig += (int64_t) 1 << (s - 1);
m_sig >>= s;
}
@@ -100,43 +100,45 @@ sreal::shift_right (int s)
void
sreal::normalize ()
{
- if (m_sig == 0)
+ int64_t s = m_sig < 0 ? -1 : 1;
+ unsigned HOST_WIDE_INT sig = absu_hwi (m_sig);
+
+ if (sig == 0)
{
- m_negative = 0;
m_exp = -SREAL_MAX_EXP;
}
- else if (m_sig < SREAL_MIN_SIG)
+ else if (sig < SREAL_MIN_SIG)
{
do
{
- m_sig <<= 1;
+ sig <<= 1;
m_exp--;
}
- while (m_sig < SREAL_MIN_SIG);
+ while (sig < SREAL_MIN_SIG);
/* Check underflow. */
if (m_exp < -SREAL_MAX_EXP)
{
m_exp = -SREAL_MAX_EXP;
- m_sig = 0;
+ sig = 0;
}
}
- else if (m_sig > SREAL_MAX_SIG)
+ else if (sig > SREAL_MAX_SIG)
{
int last_bit;
do
{
- last_bit = m_sig & 1;
- m_sig >>= 1;
+ last_bit = sig & 1;
+ sig >>= 1;
m_exp++;
}
- while (m_sig > SREAL_MAX_SIG);
+ while (sig > SREAL_MAX_SIG);
/* Round the number. */
- m_sig += last_bit;
- if (m_sig > SREAL_MAX_SIG)
+ sig += last_bit;
+ if (sig > SREAL_MAX_SIG)
{
- m_sig >>= 1;
+ sig >>= 1;
m_exp++;
}
@@ -144,9 +146,11 @@ sreal::normalize ()
if (m_exp > SREAL_MAX_EXP)
{
m_exp = SREAL_MAX_EXP;
- m_sig = SREAL_MAX_SIG;
+ sig = SREAL_MAX_SIG;
}
}
+
+ m_sig = s * sig;
}
/* Return integer value of *this. */
@@ -154,17 +158,17 @@ sreal::normalize ()
int64_t
sreal::to_int () const
{
- int64_t sign = m_negative ? -1 : 1;
+ int64_t sign = m_sig < 0 ? -1 : 1;
if (m_exp <= -SREAL_BITS)
return 0;
if (m_exp >= SREAL_PART_BITS)
return sign * INTTYPE_MAXIMUM (int64_t);
if (m_exp > 0)
- return sign * (m_sig << m_exp);
+ return m_sig << m_exp;
if (m_exp < 0)
- return sign * (m_sig >> -m_exp);
- return sign * m_sig;
+ return m_sig >> -m_exp;
+ return m_sig;
}
/* Return *this + other. */
@@ -172,36 +176,10 @@ sreal::to_int () const
sreal
sreal::operator+ (const sreal &other) const
{
- const sreal *a_p = this, *b_p = &other;
-
- if (a_p->m_negative && !b_p->m_negative)
- std::swap (a_p, b_p);
-
- /* a + -b => a - b. */
- if (!a_p->m_negative && b_p->m_negative)
- {
- sreal tmp = -(*b_p);
- if (*a_p < tmp)
- return signedless_minus (tmp, *a_p, true);
- else
- return signedless_minus (*a_p, tmp, false);
- }
-
- gcc_checking_assert (a_p->m_negative == b_p->m_negative);
-
- sreal r = signedless_plus (*a_p, *b_p, a_p->m_negative);
-
- return r;
-}
-
-sreal
-sreal::signedless_plus (const sreal &a, const sreal &b, bool negative)
-{
- const sreal *bb;
- sreal r, tmp;
int dexp;
- const sreal *a_p = &a;
- const sreal *b_p = &b;
+ sreal tmp, r;
+
+ const sreal *a_p = this, *b_p = &other, *bb;
if (a_p->m_exp < b_p->m_exp)
std::swap (a_p, b_p);
@@ -211,7 +189,6 @@ sreal::signedless_plus (const sreal &a, const sreal &b, bool negative)
if (dexp > SREAL_BITS)
{
r.m_sig = a_p->m_sig;
- r.m_negative = negative;
return r;
}
@@ -226,56 +203,32 @@ sreal::signedless_plus (const sreal &a, const sreal &b, bool negative)
r.m_sig = a_p->m_sig + bb->m_sig;
r.normalize ();
-
- r.m_negative = negative;
return r;
}
+
/* Return *this - other. */
sreal
sreal::operator- (const sreal &other) const
{
- /* -a - b => -a + (-b). */
- if (m_negative && !other.m_negative)
- return signedless_plus (*this, -other, true);
-
- /* a - (-b) => a + b. */
- if (!m_negative && other.m_negative)
- return signedless_plus (*this, -other, false);
-
- gcc_checking_assert (m_negative == other.m_negative);
-
- /* We want to substract a smaller number from bigger
- for nonegative numbers. */
- if (!m_negative && *this < other)
- return signedless_minus (other, *this, true);
-
- /* Example: -2 - (-3) => 3 - 2 */
- if (m_negative && *this > other)
- return signedless_minus (-other, -(*this), false);
-
- sreal r = signedless_minus (*this, other, m_negative);
-
- return r;
-}
-
-sreal
-sreal::signedless_minus (const sreal &a, const sreal &b, bool negative)
-{
int dexp;
sreal tmp, r;
const sreal *bb;
- const sreal *a_p = &a;
- const sreal *b_p = &b;
+ const sreal *a_p = this, *b_p = &other;
- dexp = a_p->m_exp - b_p->m_exp;
+ int64_t sign = 1;
+ if (a_p->m_exp < b_p->m_exp)
+ {
+ sign = -1;
+ std::swap (a_p, b_p);
+ }
+ dexp = a_p->m_exp - b_p->m_exp;
r.m_exp = a_p->m_exp;
if (dexp > SREAL_BITS)
{
- r.m_sig = a_p->m_sig;
- r.m_negative = negative;
+ r.m_sig = sign * a_p->m_sig;
return r;
}
if (dexp == 0)
@@ -287,10 +240,8 @@ sreal::signedless_minus (const sreal &a, const sreal &b, bool negative)
bb = &tmp;
}
- r.m_sig = a_p->m_sig - bb->m_sig;
+ r.m_sig = sign * (a_p->m_sig - bb->m_sig);
r.normalize ();
-
- r.m_negative = negative;
return r;
}
@@ -300,7 +251,7 @@ sreal
sreal::operator* (const sreal &other) const
{
sreal r;
- if (m_sig < SREAL_MIN_SIG || other.m_sig < SREAL_MIN_SIG)
+ if (std::abs (m_sig) < SREAL_MIN_SIG || std::abs (other.m_sig) < SREAL_MIN_SIG)
{
r.m_sig = 0;
r.m_exp = -SREAL_MAX_EXP;
@@ -312,7 +263,6 @@ sreal::operator* (const sreal &other) const
r.normalize ();
}
- r.m_negative = m_negative ^ other.m_negative;
return r;
}
@@ -325,7 +275,6 @@ sreal::operator/ (const sreal &other) const
sreal r;
r.m_sig = (m_sig << SREAL_PART_BITS) / other.m_sig;
r.m_exp = m_exp - other.m_exp - SREAL_PART_BITS;
- r.m_negative = m_negative ^ other.m_negative;
r.normalize ();
return r;
}
diff --git a/gcc/sreal.h b/gcc/sreal.h
index 3938c6e..730f49c 100644
--- a/gcc/sreal.h
+++ b/gcc/sreal.h
@@ -25,8 +25,8 @@ along with GCC; see the file COPYING3. If not see
#define UINT64_BITS 64
-#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 ((int64_t) 1 << (SREAL_PART_BITS - 2))
+#define SREAL_MAX_SIG (((int64_t) 1 << (SREAL_PART_BITS - 1)) - 1)
#define SREAL_MAX_EXP (INT_MAX / 4)
#define SREAL_BITS SREAL_PART_BITS
@@ -36,18 +36,11 @@ class sreal
{
public:
/* Construct an uninitialized sreal. */
- sreal () : m_sig (-1), m_exp (-1), m_negative (0) {}
+ sreal () : m_sig (-1), m_exp (-1) {}
/* Construct a sreal. */
- sreal (int64_t sig, int exp = 0) : m_exp (exp)
+ sreal (int64_t sig, int exp = 0) : m_sig (sig), m_exp (exp)
{
- m_negative = sig < 0;
-
- if (sig < 0)
- sig = -sig;
-
- m_sig = (uint64_t) sig;
-
normalize ();
}
@@ -60,33 +53,30 @@ public:
bool operator< (const sreal &other) const
{
- /* We negate result in case of negative numbers and
- it would return true for equal negative numbers. */
- if (*this == other)
- return false;
-
- if (m_negative != other.m_negative)
- return m_negative > other.m_negative;
-
- bool r = m_exp < other.m_exp
- || (m_exp == other.m_exp && m_sig < other.m_sig);
-
- return m_negative ? !r : r;
+ if (m_exp == other.m_exp)
+ return m_sig < other.m_sig;
+ else
+ {
+ bool negative = m_sig < 0;
+ bool other_negative = other.m_sig < 0;
+
+ if (negative != other_negative)
+ return negative > other_negative;
+
+ bool r = m_exp < other.m_exp;
+ return negative ? !r : r;
+ }
}
bool operator== (const sreal &other) const
{
- return m_exp == other.m_exp && m_sig == other.m_sig
- && m_negative == other.m_negative;
+ return m_exp == other.m_exp && m_sig == other.m_sig;
}
sreal operator- () const
{
- if (m_sig == 0)
- return *this;
-
sreal tmp = *this;
- tmp.m_negative = !tmp.m_negative;
+ tmp.m_sig *= -1;
return tmp;
}
@@ -125,17 +115,15 @@ public:
private:
void normalize ();
void shift_right (int amount);
-
static sreal signedless_plus (const sreal &a, const sreal &b, bool negative);
static sreal signedless_minus (const sreal &a, const sreal &b, bool negative);
- uint64_t m_sig; /* Significant. */
+ int64_t m_sig; /* Significant. */
signed int m_exp; /* Exponent. */
- bool m_negative; /* Negative sign. */
};
-extern void debug (sreal &ref);
-extern void debug (sreal *ptr);
+extern void debug (const sreal &ref);
+extern void debug (const sreal *ptr);
inline sreal &operator+= (sreal &a, const sreal &b)
{
--
2.1.2
