diff mbox

Fix sreal::shift add to_double conversion

Message ID 20141204180012.GC16755@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka Dec. 4, 2014, 6 p.m. UTC 
Hi,
this patch fixes implementation of shift in sreal.h that ICEs on 0 value.
Also the comment is copied from shift_right and does not make sense for
sreal.h.

The to_double conversion is useful for debug output: Most of inliner data is
now output as integer that does not make much sense.

Bootstrapped/regtested x86_64-linux, OK?

	* sreal.h (to_double): Declare.
	(shift): Do not ICE on 0, update outdated comment.
	* sreal.c: Include math.h
	(sreal::to_double): New function.

Comments

Richard Biener Dec. 4, 2014, 7:12 p.m. UTC | #1 
On December 4, 2014 7:00:12 PM CET, Jan Hubicka <hubicka@ucw.cz> wrote:
>Hi,
>this patch fixes implementation of shift in sreal.h that ICEs on 0
>value.
>Also the comment is copied from shift_right and does not make sense for
>sreal.h.
>
>The to_double conversion is useful for debug output: Most of inliner
>data is
>now output as integer that does not make much sense.

Can't you use dump for this?

Richard.

>Bootstrapped/regtested x86_64-linux, OK?
>
>	* sreal.h (to_double): Declare.
>	(shift): Do not ICE on 0, update outdated comment.
>	* sreal.c: Include math.h
>	(sreal::to_double): New function.
>Index: sreal.h
>===================================================================
>--- sreal.h	(revision 218286)
>+++ sreal.h	(working copy)
>@@ -53,6 +53,7 @@
> 
>   void dump (FILE *) const;
>   int64_t to_int () const;
>+  double to_double () const;
>   sreal operator+ (const sreal &other) const;
>   sreal operator- (const sreal &other) const;
>   sreal operator* (const sreal &other) const;
>@@ -93,12 +94,14 @@
> 
>   sreal shift (int s) const
>   {
>+    /* Zero needs no shifting.  */
>+    if (!m_sig)
>+      return *this;
>     gcc_checking_assert (s <= SREAL_BITS);
>     gcc_checking_assert (s >= -SREAL_BITS);
> 
>-    /* Exponent should never be so large because shift_right is used
>only by
>-     sreal_add and sreal_sub ant thus the number cannot be shifted out
>from
>-     exponent range.  */
>+    /* Overflows/drop to 0 could be handled gracefully, but hopefully
>we do not
>+       need to do so.  */
>     gcc_checking_assert (m_exp + s <= SREAL_MAX_EXP);
>     gcc_checking_assert (m_exp + s >= -SREAL_MAX_EXP);
> 
>Index: sreal.c
>===================================================================
>--- sreal.c	(revision 218286)
>+++ sreal.c	(working copy)
>@@ -47,6 +47,7 @@
> 	sig == 0 && exp == -SREAL_MAX_EXP
> */
> 
>+#include <math.h>
> #include "config.h"
> #include "system.h"
> #include "coretypes.h"
>@@ -167,6 +168,19 @@
>   return sign * m_sig;
> }
> 
>+/* Return value of *this as double.  */
>+
>+double
>+sreal::to_double () const
>+{
>+  double val = m_sig;
>+  if (m_negative)
>+    val = -val;
>+  if (m_exp)
>+    val *= exp2 (m_exp);
>+  return val;
>+}
>+
> /* Return *this + other.  */
> 
> sreal
Jan Hubicka Dec. 4, 2014, 8:06 p.m. UTC | #2 
> On December 4, 2014 7:00:12 PM CET, Jan Hubicka <hubicka@ucw.cz> wrote:
> >Hi,
> >this patch fixes implementation of shift in sreal.h that ICEs on 0
> >value.
> >Also the comment is copied from shift_right and does not make sense for
> >sreal.h.
> >
> >The to_double conversion is useful for debug output: Most of inliner
> >data is
> >now output as integer that does not make much sense.
> 
> Can't you use dump for this?

Well, dump will get you somewhat less readable form like 1234*2^3 and also it makes it hard to dump
multiple values with single printf.  
      if (dump)
        {
          sreal num,den;
          relative_time_benefit (callee_info, edge, edge_time, &num, &den);
          fprintf (dump_file,
                   "      %f: guessed profile. frequency %f,"
                   " benefit %f%%, time w/o inlining %f, time w inlining %f"
                   " overall growth %i (current) %i (original)\n",
                   badness.to_double (), (double)edge->frequency / CGRAPH_FREQ_BASE,
                   (num/den).to_double () * 100,
                   compute_uninlined_call_time (callee_info, edge).to_double (),
                   compute_inlined_call_time (edge, edge_time).to_double (),
                   estimate_growth (callee),
                   callee_info->growth);
        }

would look even more ugly if done with dump method.

Honza
> 
> Richard.
> 
> >Bootstrapped/regtested x86_64-linux, OK?
> >
> >	* sreal.h (to_double): Declare.
> >	(shift): Do not ICE on 0, update outdated comment.
> >	* sreal.c: Include math.h
> >	(sreal::to_double): New function.
> >Index: sreal.h
> >===================================================================
> >--- sreal.h	(revision 218286)
> >+++ sreal.h	(working copy)
> >@@ -53,6 +53,7 @@
> > 
> >   void dump (FILE *) const;
> >   int64_t to_int () const;
> >+  double to_double () const;
> >   sreal operator+ (const sreal &other) const;
> >   sreal operator- (const sreal &other) const;
> >   sreal operator* (const sreal &other) const;
> >@@ -93,12 +94,14 @@
> > 
> >   sreal shift (int s) const
> >   {
> >+    /* Zero needs no shifting.  */
> >+    if (!m_sig)
> >+      return *this;
> >     gcc_checking_assert (s <= SREAL_BITS);
> >     gcc_checking_assert (s >= -SREAL_BITS);
> > 
> >-    /* Exponent should never be so large because shift_right is used
> >only by
> >-     sreal_add and sreal_sub ant thus the number cannot be shifted out
> >from
> >-     exponent range.  */
> >+    /* Overflows/drop to 0 could be handled gracefully, but hopefully
> >we do not
> >+       need to do so.  */
> >     gcc_checking_assert (m_exp + s <= SREAL_MAX_EXP);
> >     gcc_checking_assert (m_exp + s >= -SREAL_MAX_EXP);
> > 
> >Index: sreal.c
> >===================================================================
> >--- sreal.c	(revision 218286)
> >+++ sreal.c	(working copy)
> >@@ -47,6 +47,7 @@
> > 	sig == 0 && exp == -SREAL_MAX_EXP
> > */
> > 
> >+#include <math.h>
> > #include "config.h"
> > #include "system.h"
> > #include "coretypes.h"
> >@@ -167,6 +168,19 @@
> >   return sign * m_sig;
> > }
> > 
> >+/* Return value of *this as double.  */
> >+
> >+double
> >+sreal::to_double () const
> >+{
> >+  double val = m_sig;
> >+  if (m_negative)
> >+    val = -val;
> >+  if (m_exp)
> >+    val *= exp2 (m_exp);
> >+  return val;
> >+}
> >+
> > /* Return *this + other.  */
> > 
> > sreal
>
Richard Biener Dec. 8, 2014, 9:21 a.m. UTC | #3 
On Thu, Dec 4, 2014 at 9:06 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On December 4, 2014 7:00:12 PM CET, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >Hi,
>> >this patch fixes implementation of shift in sreal.h that ICEs on 0
>> >value.
>> >Also the comment is copied from shift_right and does not make sense for
>> >sreal.h.
>> >
>> >The to_double conversion is useful for debug output: Most of inliner
>> >data is
>> >now output as integer that does not make much sense.
>>
>> Can't you use dump for this?
>
> Well, dump will get you somewhat less readable form like 1234*2^3 and also it makes it hard to dump
> multiple values with single printf.
>       if (dump)
>         {
>           sreal num,den;
>           relative_time_benefit (callee_info, edge, edge_time, &num, &den);
>           fprintf (dump_file,
>                    "      %f: guessed profile. frequency %f,"
>                    " benefit %f%%, time w/o inlining %f, time w inlining %f"
>                    " overall growth %i (current) %i (original)\n",
>                    badness.to_double (), (double)edge->frequency / CGRAPH_FREQ_BASE,
>                    (num/den).to_double () * 100,
>                    compute_uninlined_call_time (callee_info, edge).to_double (),
>                    compute_inlined_call_time (edge, edge_time).to_double (),
>                    estimate_growth (callee),
>                    callee_info->growth);
>         }
>
> would look even more ugly if done with dump method.

Hmm, ok.  Please add a comment before to_double that it is only
supposed to be used for debug dumping.

Richard.

> Honza
>>
>> Richard.
>>
>> >Bootstrapped/regtested x86_64-linux, OK?
>> >
>> >     * sreal.h (to_double): Declare.
>> >     (shift): Do not ICE on 0, update outdated comment.
>> >     * sreal.c: Include math.h
>> >     (sreal::to_double): New function.
>> >Index: sreal.h
>> >===================================================================
>> >--- sreal.h  (revision 218286)
>> >+++ sreal.h  (working copy)
>> >@@ -53,6 +53,7 @@
>> >
>> >   void dump (FILE *) const;
>> >   int64_t to_int () const;
>> >+  double to_double () const;
>> >   sreal operator+ (const sreal &other) const;
>> >   sreal operator- (const sreal &other) const;
>> >   sreal operator* (const sreal &other) const;
>> >@@ -93,12 +94,14 @@
>> >
>> >   sreal shift (int s) const
>> >   {
>> >+    /* Zero needs no shifting.  */
>> >+    if (!m_sig)
>> >+      return *this;
>> >     gcc_checking_assert (s <= SREAL_BITS);
>> >     gcc_checking_assert (s >= -SREAL_BITS);
>> >
>> >-    /* Exponent should never be so large because shift_right is used
>> >only by
>> >-     sreal_add and sreal_sub ant thus the number cannot be shifted out
>> >from
>> >-     exponent range.  */
>> >+    /* Overflows/drop to 0 could be handled gracefully, but hopefully
>> >we do not
>> >+       need to do so.  */
>> >     gcc_checking_assert (m_exp + s <= SREAL_MAX_EXP);
>> >     gcc_checking_assert (m_exp + s >= -SREAL_MAX_EXP);
>> >
>> >Index: sreal.c
>> >===================================================================
>> >--- sreal.c  (revision 218286)
>> >+++ sreal.c  (working copy)
>> >@@ -47,6 +47,7 @@
>> >     sig == 0 && exp == -SREAL_MAX_EXP
>> > */
>> >
>> >+#include <math.h>
>> > #include "config.h"
>> > #include "system.h"
>> > #include "coretypes.h"
>> >@@ -167,6 +168,19 @@
>> >   return sign * m_sig;
>> > }
>> >
>> >+/* Return value of *this as double.  */
>> >+
>> >+double
>> >+sreal::to_double () const
>> >+{
>> >+  double val = m_sig;
>> >+  if (m_negative)
>> >+    val = -val;
>> >+  if (m_exp)
>> >+    val *= exp2 (m_exp);
>> >+  return val;
>> >+}
>> >+
>> > /* Return *this + other.  */
>> >
>> > sreal
>>
diff mbox

Patch

Index: sreal.h
===================================================================
--- sreal.h	(revision 218286)
+++ sreal.h	(working copy)
@@ -53,6 +53,7 @@ 
 
   void dump (FILE *) const;
   int64_t to_int () const;
+  double to_double () const;
   sreal operator+ (const sreal &other) const;
   sreal operator- (const sreal &other) const;
   sreal operator* (const sreal &other) const;
@@ -93,12 +94,14 @@ 
 
   sreal shift (int s) const
   {
+    /* Zero needs no shifting.  */
+    if (!m_sig)
+      return *this;
     gcc_checking_assert (s <= SREAL_BITS);
     gcc_checking_assert (s >= -SREAL_BITS);
 
-    /* Exponent should never be so large because shift_right is used only by
-     sreal_add and sreal_sub ant thus the number cannot be shifted out from
-     exponent range.  */
+    /* Overflows/drop to 0 could be handled gracefully, but hopefully we do not
+       need to do so.  */
     gcc_checking_assert (m_exp + s <= SREAL_MAX_EXP);
     gcc_checking_assert (m_exp + s >= -SREAL_MAX_EXP);
 
Index: sreal.c
===================================================================
--- sreal.c	(revision 218286)
+++ sreal.c	(working copy)
@@ -47,6 +47,7 @@ 
 	sig == 0 && exp == -SREAL_MAX_EXP
 */
 
+#include <math.h>
 #include "config.h"
 #include "system.h"
 #include "coretypes.h"
@@ -167,6 +168,19 @@ 
   return sign * m_sig;
 }
 
+/* Return value of *this as double.  */
+
+double
+sreal::to_double () const
+{
+  double val = m_sig;
+  if (m_negative)
+    val = -val;
+  if (m_exp)
+    val *= exp2 (m_exp);
+  return val;
+}
+
 /* Return *this + other.  */
 
 sreal