On Fri, Nov 14, 2014 at 12:40:23AM +0100, Jan Hubicka wrote:
> Hello,
> in general I like this addition - I was not aware that sreal has no support
> for negative values.
> This would be serious maintainance burden if sreals was used for profile
> updating - it is very
> easy to get negative temporaries while dong the updates.
> > gcc/ChangeLog:
> >
> > 2014-11-13 Martin Liska <[email protected]>
> >
> > * predict.c (propagate_freq): More elegant sreal API is used.
> > (estimate_bb_frequencies): New static constants defined by sreal
> > replace precomputed ones.
> > * sreal.c (sreal::normalize): New function.
> > (sreal::to_int): Likewise.
> > (sreal::operator+): Likewise.
> > (sreal::operator-): Likewise.
> > * sreal.h: Definition of new functions added.
> > diff --git a/gcc/sreal.c b/gcc/sreal.c
> > index 3f5456a..89b9c4d 100644
> > --- a/gcc/sreal.c
> > +++ b/gcc/sreal.c
> > @@ -1,4 +1,4 @@
> > -/* Simple data type for positive real numbers for the GNU compiler.
> > +/* Simple data type for real numbers for the GNU compiler.
> > Copyright (C) 2002-2014 Free Software Foundation, Inc.
> >
> > This file is part of GCC.
> > @@ -17,7 +17,7 @@ You should have received a copy of the GNU General Public
> > License
> > along with GCC; see the file COPYING3. If not see
> > <http://www.gnu.org/licenses/>. */
> >
> > -/* This library supports positive real numbers and 0;
> > +/* This library supports real numbers;
> > inf and nan are NOT supported.
> > It is written to be simple and fast.
> >
> > @@ -102,6 +102,7 @@ sreal::normalize ()
> > {
> > if (m_sig == 0)
> > {
> > + m_negative = 0;
> > m_exp = -SREAL_MAX_EXP;
> > }
> > else if (m_sig < SREAL_MIN_SIG)
> > @@ -153,15 +154,17 @@ sreal::normalize ()
> > int64_t
> > sreal::to_int () const
> > {
> > + int64_t sign = m_negative ? -1 : 1;
> > +
> > if (m_exp <= -SREAL_BITS)
> > return 0;
> > if (m_exp >= SREAL_PART_BITS)
> > - return INTTYPE_MAXIMUM (int64_t);
> > + return sign * INTTYPE_MAXIMUM (int64_t);
> > if (m_exp > 0)
> > - return m_sig << m_exp;
> > + return sign * (m_sig << m_exp);
> > if (m_exp < 0)
> > - return m_sig >> -m_exp;
> > - return m_sig;
> > + return sign * (m_sig >> -m_exp);
> > + return sign * m_sig;
> > }
> >
> > /* Return *this + other. */
> > @@ -169,9 +172,19 @@ sreal::to_int () const
> > sreal
> > sreal::operator+ (const sreal &other) const
> > {
> > + const sreal *a_p = this, *b_p = &other, *bb;
> > +
> > + if (m_negative && !other.m_negative)
> > + return other + *a_p;
> > +
> > + if (!m_negative && other.m_negative)
> > + return *a_p - -other;
> > +
> > + gcc_assert (m_negative == other.m_negative);
>
> I wonder what kind of code this winds into - you need recursive inlining to
> fix
> this and it won't be clear to inliner that the recursion depth is 1. Perhaps
maybe tail recurse optimizer is smart enough to see it can use a jump,
and then it might not be too bad.
> having an inline private function for signedless + and - and implementing real
> operators by this would factor the code better.
>
> I wonder if there is not a faster implementation given that this code is on
> way to internal loops of inliner. If significand was signed, some of these
> operations
> would just work, right?
I believe so, but I haven't actually proved it to myself (I also
suggested going this route).
I'll try and look in this in more depth, but so many things to do and a
bunch of my time the next few days will be at family events.
Trev
> > diff --git a/gcc/sreal.h b/gcc/sreal.h
> > index 461e28b..bfed3c7 100644
> > --- a/gcc/sreal.h
> > +++ b/gcc/sreal.h
> > @@ -1,4 +1,4 @@
> > -/* Definitions for simple data type for positive real numbers.
> > +/* Definitions for simple data type for real numbers.
> > Copyright (C) 2002-2014 Free Software Foundation, Inc.
> >
> > This file is part of GCC.
> > @@ -25,7 +25,7 @@ along with GCC; see the file COPYING3. If not see
> >
> > #define SREAL_MIN_SIG ((uint64_t) 1 << (SREAL_PART_BITS - 1))
> > #define SREAL_MAX_SIG (((uint64_t) 1 << SREAL_PART_BITS) - 1)
> > -#define SREAL_MAX_EXP (INT_MAX / 4)
> > +#define SREAL_MAX_EXP (INT_MAX / 8)
> >
> > #define SREAL_BITS SREAL_PART_BITS
> >
> > @@ -34,10 +34,21 @@ class sreal
> > {
> > public:
> > /* Construct an uninitialized sreal. */
> > - sreal () : m_sig (-1), m_exp (-1) {}
> > + sreal () : m_sig (-1), m_exp (-1), m_negative (0) {}
> >
> > /* Construct a sreal. */
> > - sreal (uint64_t sig, int exp) : m_sig (sig), m_exp (exp) { normalize ();
> > }
> > + sreal (int64_t sig, int exp = 0) : m_exp (exp)
> > + {
> > + m_negative = sig < 0;
> > +
> > + // TODO: speed up
> I think we do not need to care about this; expression folding should
> > + if (sig < 0)
> > + sig = -sig;
> > +
> > + m_sig = (uint64_t) sig;
> > +
> > + normalize ();
> > + }
>
> We probably want sreal conversion from both signed and unsigned ints?
> For debugging output it would be very useful to also have conversion to
> double.
> The dump method is bit unhandy to use.
> >
> > void dump (FILE *) const;
> > int64_t to_int () const;
> > @@ -49,13 +60,58 @@ public:
> >
> > bool operator< (const sreal &other) const
> > {
> > - return m_exp < other.m_exp
> > + if (m_negative != other.m_negative)
> > + return m_negative > other.m_negative;
> I guess negative zero is smaller than positive, but I do not see why that
> would be problem
>
> Again, I would preffer C++ person to glance over this.
>
> Honza
