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

[Jeff Law]

On 11/13/14 05:35, mliska wrote:
gcc/ChangeLog:
2014-11-13  Martin Liska  <mli...@suse.cz>

        * 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.
Shouldn't this be moving forward independent of the fibheap changes?


---
  gcc/predict.c | 30 +++++++++++-------------
  gcc/sreal.c   | 56 ++++++++++++++++++++++++++++++++++++--------
  gcc/sreal.h   | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------
  3 files changed, 126 insertions(+), 35 deletions(-)

diff --git a/gcc/predict.c b/gcc/predict.c
index 0215e91..0f640f5 100644
--- a/gcc/predict.c
+++ b/gcc/predict.c
@@ -82,7 +82,7 @@ along with GCC; see the file COPYING3.  If not see

  /* real constants: 0, 1, 1-1/REG_BR_PROB_BASE, REG_BR_PROB_BASE,
                   1/REG_BR_PROB_BASE, 0.5, BB_FREQ_MAX.  */
-static sreal real_zero, real_one, real_almost_one, real_br_prob_base,
+static sreal real_almost_one, real_br_prob_base,
             real_inv_br_prob_base, real_one_half, real_bb_freq_max;
Update comment to reflect the remaining values.

Implementation of the arithmetics seems sane in the sense that we're 
just using basic identities to put everything into a canonical form then 
calling back into one of the existing functions.

If this is supposed to be small and fast, do we pay a noticeable penalty 
for canonicalizing then another call?  Would we be better off (for 
example) directly handling a + b with a negative and b positive?

I think it's OK to assume we can apply these identities as we're 
building a minimalist real implementation.   Obviously if we were 
building an IEEE compliant implementation, this would all be 
considerably more complex ;-)



@@ -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
+    if (sig < 0)
+      sig = -sig;
Not sure what you really need to do here.  You could test and flip the 
sign bit, but  I'm pretty sure that's already handled for you in GCC, at 
least in the cases that are likely to matter.

So one thing that comes to mind is that it looks like we have +-0.  THey 
compare unequal and have an ordering.  Not sure if that's desirable or not.

I assume the shift method is used by one of the other patches in the 
series.  RIght?

So with the comment fix, some comment about handling of +-0, I think 
this is good to go and can go in independently of the rest of the patch 
series.

jeff