diff mbox

[RFC,13/19] Target FP: Perform Ada fixed-point scaling in target format

Message ID 20170905182135.5FF0AD8086F@oc3748833570.ibm.com
State New
Headers show

Commit Message

Ulrich Weigand Sept. 5, 2017, 6:21 p.m. UTC
[RFC][13/19] Target FP: Perform Ada fixed-point scaling in target format

One of the few still remaining uses of DOUBLEST in GDB is the Ada front-end
code that handles scaling of Ada fixed-point types.  The target format for
those types is some integer format; to convert those values to standard
floating-point representation, that integer needs to be multiplied by a
rational scale factor, given as a pair of numerator and denominator.

To avoid having to deal with long integer arithmetic, the current Ada
front-end code currently performs those scaling operations in host
DOUBLEST arithmetic.  To eliminate this use of DOUBLEST, this patch
changes the front-end to instead perform those operations in the
*target* floating-point format (chosing to use the target "long double").

The implementation is mostly straight-forward, using value_cast and
value_binop to perform the target operations.

Scanning in the scale numerator and denominator is now done into
a host "long long" instead of a DOUBLEST, which should be large
enough to hold all possible values.  (Otherwise, this can be replaced
by target-format target_float_from_string operations as well.)

Printing fixed-point types and values should be completely unchanges,
using target_float_to_string with the same format strings as current code.

Bye,
Ulrich

ChangeLog:

	* ada-lang.c (cast_to_fixed): Reimplement in target arithmetic.
	(cast_to_fixed): Likewise.
	(ada_scaling_type): New function.
	(ada_delta): Return value instead of DOUBLEST.  Perform target
	arithmetic instead of host arithmetic.
	(scaling_factor): Rename to ...
	(ada_scaling_factor) ... this.  Return value instead of DOUBLEST.
	Perform target arithmetic instead of host arithmetic.
	(ada_fixed_to_float): Remove.
	(ada_float_to_fixed): Remove.
	* ada-lang.h (ada_fixed_to_float): Remove.
	(ada_float_to_fixed): Remove.
	(ada_delta): Return value instead of DOUBLEST.
	(ada_scaling_factor): Add prototype.

	* ada-typeprint.c: Include "target-float.h".
	(print_fixed_point_type): Perform target arithmetic instead of
	host arithmetic.
	* ada-valprint.c: Include "target-float.h".
	(ada_val_print_num): Perform target arithmetic instead of
	host arithmetic for fixed-point types.

Comments

Joel Brobecker Oct. 9, 2017, 4:30 p.m. UTC | #1
On Tue, Sep 05, 2017 at 08:21:35PM +0200, Ulrich Weigand wrote:
> [RFC][13/19] Target FP: Perform Ada fixed-point scaling in target format
> 
> One of the few still remaining uses of DOUBLEST in GDB is the Ada front-end
> code that handles scaling of Ada fixed-point types.  The target format for
> those types is some integer format; to convert those values to standard
> floating-point representation, that integer needs to be multiplied by a
> rational scale factor, given as a pair of numerator and denominator.
> 
> To avoid having to deal with long integer arithmetic, the current Ada
> front-end code currently performs those scaling operations in host
> DOUBLEST arithmetic.  To eliminate this use of DOUBLEST, this patch
> changes the front-end to instead perform those operations in the
> *target* floating-point format (chosing to use the target "long double").
> 
> The implementation is mostly straight-forward, using value_cast and
> value_binop to perform the target operations.
> 
> Scanning in the scale numerator and denominator is now done into
> a host "long long" instead of a DOUBLEST, which should be large
> enough to hold all possible values.  (Otherwise, this can be replaced
> by target-format target_float_from_string operations as well.)
> 
> Printing fixed-point types and values should be completely unchanges,
> using target_float_to_string with the same format strings as current code.
> 
> Bye,
> Ulrich
> 
> ChangeLog:
> 
> 	* ada-lang.c (cast_to_fixed): Reimplement in target arithmetic.
> 	(cast_to_fixed): Likewise.
         ^^^^^^^^^^^^^
         cast_from_fixed :)

> 	(ada_scaling_type): New function.
> 	(ada_delta): Return value instead of DOUBLEST.  Perform target
> 	arithmetic instead of host arithmetic.
> 	(scaling_factor): Rename to ...
> 	(ada_scaling_factor) ... this.  Return value instead of DOUBLEST.
> 	Perform target arithmetic instead of host arithmetic.

Maybe mention that we are making this function non-static too?

> 	(ada_fixed_to_float): Remove.
> 	(ada_float_to_fixed): Remove.
> 	* ada-lang.h (ada_fixed_to_float): Remove.
> 	(ada_float_to_fixed): Remove.
> 	(ada_delta): Return value instead of DOUBLEST.
> 	(ada_scaling_factor): Add prototype.
> 
> 	* ada-typeprint.c: Include "target-float.h".
> 	(print_fixed_point_type): Perform target arithmetic instead of
> 	host arithmetic.
> 	* ada-valprint.c: Include "target-float.h".
> 	(ada_val_print_num): Perform target arithmetic instead of
> 	host arithmetic for fixed-point types.

Generally speaking, the patch looks good to me, and the only remark
I have is actually an trivial C++ question which you probably had
covered.

We have a fixed point tests in the testsuite (gdb.ada/fixed_points.exp),
so having it run and pass after your change should be a very good sanity
check on its own.

Thanks for doing that!

[...]
> Index: binutils-gdb/gdb/ada-typeprint.c
> ===================================================================
> --- binutils-gdb.orig/gdb/ada-typeprint.c
> +++ binutils-gdb/gdb/ada-typeprint.c
[...]
> @@ -360,16 +361,23 @@ print_enum_type (struct type *type, stru
>  static void
>  print_fixed_point_type (struct type *type, struct ui_file *stream)
>  {
> -  DOUBLEST delta = ada_delta (type);
> -  DOUBLEST small = ada_fixed_to_float (type, 1);
> +  struct value *delta = ada_delta (type);
> +  struct value *small = ada_scaling_factor (type);
>  
> -  if (delta < 0.0)
> +  if (delta == nullptr)
>      fprintf_filtered (stream, "delta ??");
>    else
>      {
> -      fprintf_filtered (stream, "delta %g", (double) delta);
> -      if (delta != small)
> -	fprintf_filtered (stream, " <'small = %g>", (double) small);
> +      std::string str;
> +      str = target_float_to_string (value_contents (delta),
> +				    value_type (delta), "%g");

This is a C++ question, really. Does it make any difference if you
declare the std::string first, and then only set its contents in
a second statement? I can't remember the details, but it has to do
with initialization vs assignment. I _really_ hope that the string
class is sufficiently well designed that the two are really equivalent
in practice?
Joel Brobecker Oct. 9, 2017, 6:09 p.m. UTC | #2
> > This is a C++ question, really. Does it make any difference if you
> > declare the std::string first, and then only set its contents in
> > a second statement? I can't remember the details, but it has to do
> > with initialization vs assignment. I _really_ hope that the string
> > class is sufficiently well designed that the two are really equivalent
> > in practice?
> 
> Huh.  Indeed I see worse code when doing the assignment as a separate
> statement, at least with GCC 4.8.  I'll make sure to use initialization
> instead wherever possible.  Thanks for pointing this out!

Let's wait for people who really know better about C++ to tell us
whether it makes a difference. I was amazed as how careful you have
to be when using C++ to avoid inefficiencies, but perhaps I am simply
being paranoid in this case... That's why I tried to phrase this as
a question.
Simon Marchi Oct. 9, 2017, 9:12 p.m. UTC | #3
On 2017-10-09 02:09 PM, Joel Brobecker wrote:
>>> This is a C++ question, really. Does it make any difference if you
>>> declare the std::string first, and then only set its contents in
>>> a second statement? I can't remember the details, but it has to do
>>> with initialization vs assignment. I _really_ hope that the string
>>> class is sufficiently well designed that the two are really equivalent
>>> in practice?
>>
>> Huh.  Indeed I see worse code when doing the assignment as a separate
>> statement, at least with GCC 4.8.  I'll make sure to use initialization
>> instead wherever possible.  Thanks for pointing this out!
> 
> Let's wait for people who really know better about C++ to tell us
> whether it makes a difference. I was amazed as how careful you have
> to be when using C++ to avoid inefficiencies, but perhaps I am simply
> being paranoid in this case... That's why I tried to phrase this as
> a question.
> 

Indeed, it's preferable to use

  std::string foo = returns_string ();

than

  std::string foo;
  foo = returns_string ();

The reason being that in the second case, the default constructor (the
one with no params) is called to construct an empty string, and then that
work is scrapped because we assign a new value.  The first form constructs
the string right away with the right content.

While it's preferable to use the first one, the earth wouldn't stop spinning
if you used the second form.  In the particular case of std::string, the
default constructor is basically assigning a few fields.

Simon
Maciej W. Rozycki Oct. 11, 2017, 5:49 p.m. UTC | #4
On Mon, 9 Oct 2017, Simon Marchi wrote:

> > Let's wait for people who really know better about C++ to tell us
> > whether it makes a difference. I was amazed as how careful you have
> > to be when using C++ to avoid inefficiencies, but perhaps I am simply
> > being paranoid in this case... That's why I tried to phrase this as
> > a question.
> > 
> 
> Indeed, it's preferable to use
> 
>   std::string foo = returns_string ();
> 
> than
> 
>   std::string foo;
>   foo = returns_string ();
> 
> The reason being that in the second case, the default constructor (the
> one with no params) is called to construct an empty string, and then that
> work is scrapped because we assign a new value.  The first form constructs
> the string right away with the right content.

 Hmm, wouldn't a half-decent compiler notice that the result produced by 
the default constructor is discarded and optimise the call away?  Also has 
there actually been an assertion in (the current definition of C++) that 
all declared objects have also been initialised?

  Maciej the curious
diff mbox

Patch

Index: binutils-gdb/gdb/ada-lang.c
===================================================================
--- binutils-gdb.orig/gdb/ada-lang.c
+++ binutils-gdb/gdb/ada-lang.c
@@ -9566,33 +9566,29 @@  unwrap_value (struct value *val)
 }
 
 static struct value *
-cast_to_fixed (struct type *type, struct value *arg)
+cast_from_fixed (struct type *type, struct value *arg)
 {
-  LONGEST val;
-
-  if (type == value_type (arg))
-    return arg;
-  else if (ada_is_fixed_point_type (value_type (arg)))
-    val = ada_float_to_fixed (type,
-                              ada_fixed_to_float (value_type (arg),
-                                                  value_as_long (arg)));
-  else
-    {
-      DOUBLEST argd = value_as_double (arg);
-
-      val = ada_float_to_fixed (type, argd);
-    }
+  struct value *scale = ada_scaling_factor (value_type (arg));
+  arg = value_cast (value_type (scale), arg);
 
-  return value_from_longest (type, val);
+  arg = value_binop (arg, scale, BINOP_MUL);
+  return value_cast (type, arg);
 }
 
 static struct value *
-cast_from_fixed (struct type *type, struct value *arg)
+cast_to_fixed (struct type *type, struct value *arg)
 {
-  DOUBLEST val = ada_fixed_to_float (value_type (arg),
-                                     value_as_long (arg));
+  if (type == value_type (arg))
+    return arg;
+
+  struct value *scale = ada_scaling_factor (type);
+  if (ada_is_fixed_point_type (value_type (arg)))
+    arg = cast_from_fixed (value_type (scale), arg);
+  else
+    arg = value_cast (value_type (scale), arg);
 
-  return value_from_double (type, val);
+  arg = value_binop (arg, scale, BINOP_DIV);
+  return value_cast (type, arg);
 }
 
 /* Given two array types T1 and T2, return nonzero iff both arrays
@@ -11474,68 +11470,57 @@  ada_is_system_address_type (struct type
 }
 
 /* Assuming that TYPE is the representation of an Ada fixed-point
-   type, return its delta, or -1 if the type is malformed and the
+   type, return the target floating-point type to be used to represent
+   of this type during internal computation.  */
+
+static struct type *
+ada_scaling_type (struct type *type)
+{
+  return builtin_type (get_type_arch (type))->builtin_long_double;
+}
+
+/* Assuming that TYPE is the representation of an Ada fixed-point
+   type, return its delta, or NULL if the type is malformed and the
    delta cannot be determined.  */
 
-DOUBLEST
+struct value *
 ada_delta (struct type *type)
 {
   const char *encoding = fixed_type_info (type);
-  DOUBLEST num, den;
+  struct type *scale_type = ada_scaling_type (type);
 
-  /* Strictly speaking, num and den are encoded as integer.  However,
-     they may not fit into a long, and they will have to be converted
-     to DOUBLEST anyway.  So scan them as DOUBLEST.  */
-  if (sscanf (encoding, "_%" DOUBLEST_SCAN_FORMAT "_%" DOUBLEST_SCAN_FORMAT,
-	      &num, &den) < 2)
-    return -1.0;
+  long long num, den;
+
+  if (sscanf (encoding, "_%lld_%lld", &num, &den) < 2)
+    return nullptr;
   else
-    return num / den;
+    return value_binop (value_from_longest (scale_type, num),
+			value_from_longest (scale_type, den), BINOP_DIV);
 }
 
 /* Assuming that ada_is_fixed_point_type (TYPE), return the scaling
    factor ('SMALL value) associated with the type.  */
 
-static DOUBLEST
-scaling_factor (struct type *type)
+struct value *
+ada_scaling_factor (struct type *type)
 {
   const char *encoding = fixed_type_info (type);
-  DOUBLEST num0, den0, num1, den1;
+  struct type *scale_type = ada_scaling_type (type);
+
+  long long num0, den0, num1, den1;
   int n;
 
-  /* Strictly speaking, num's and den's are encoded as integer.  However,
-     they may not fit into a long, and they will have to be converted
-     to DOUBLEST anyway.  So scan them as DOUBLEST.  */
-  n = sscanf (encoding,
-	      "_%" DOUBLEST_SCAN_FORMAT "_%" DOUBLEST_SCAN_FORMAT
-	      "_%" DOUBLEST_SCAN_FORMAT "_%" DOUBLEST_SCAN_FORMAT,
+  n = sscanf (encoding, "_%lld_%lld_%lld_%lld",
 	      &num0, &den0, &num1, &den1);
 
   if (n < 2)
-    return 1.0;
+    return value_from_longest (scale_type, 1);
   else if (n == 4)
-    return num1 / den1;
+    return value_binop (value_from_longest (scale_type, num1),
+			value_from_longest (scale_type, den1), BINOP_DIV);
   else
-    return num0 / den0;
-}
-
-
-/* Assuming that X is the representation of a value of fixed-point
-   type TYPE, return its floating-point equivalent.  */
-
-DOUBLEST
-ada_fixed_to_float (struct type *type, LONGEST x)
-{
-  return (DOUBLEST) x *scaling_factor (type);
-}
-
-/* The representation of a fixed-point value of type TYPE
-   corresponding to the value X.  */
-
-LONGEST
-ada_float_to_fixed (struct type *type, DOUBLEST x)
-{
-  return (LONGEST) (x / scaling_factor (type) + 0.5);
+    return value_binop (value_from_longest (scale_type, num0),
+			value_from_longest (scale_type, den0), BINOP_DIV);
 }
 
 
Index: binutils-gdb/gdb/ada-lang.h
===================================================================
--- binutils-gdb.orig/gdb/ada-lang.h
+++ binutils-gdb/gdb/ada-lang.h
@@ -305,11 +305,9 @@  extern int ada_is_fixed_point_type (stru
 
 extern int ada_is_system_address_type (struct type *);
 
-extern DOUBLEST ada_delta (struct type *);
+extern struct value *ada_delta (struct type *);
 
-extern DOUBLEST ada_fixed_to_float (struct type *, LONGEST);
-
-extern LONGEST ada_float_to_fixed (struct type *, DOUBLEST);
+extern struct value *ada_scaling_factor (struct type *);
 
 extern struct type *ada_system_address_type (void);
 
Index: binutils-gdb/gdb/ada-typeprint.c
===================================================================
--- binutils-gdb.orig/gdb/ada-typeprint.c
+++ binutils-gdb/gdb/ada-typeprint.c
@@ -31,6 +31,7 @@ 
 #include "demangle.h"
 #include "c-lang.h"
 #include "typeprint.h"
+#include "target-float.h"
 #include "ada-lang.h"
 #include <ctype.h>
 
@@ -360,16 +361,23 @@  print_enum_type (struct type *type, stru
 static void
 print_fixed_point_type (struct type *type, struct ui_file *stream)
 {
-  DOUBLEST delta = ada_delta (type);
-  DOUBLEST small = ada_fixed_to_float (type, 1);
+  struct value *delta = ada_delta (type);
+  struct value *small = ada_scaling_factor (type);
 
-  if (delta < 0.0)
+  if (delta == nullptr)
     fprintf_filtered (stream, "delta ??");
   else
     {
-      fprintf_filtered (stream, "delta %g", (double) delta);
-      if (delta != small)
-	fprintf_filtered (stream, " <'small = %g>", (double) small);
+      std::string str;
+      str = target_float_to_string (value_contents (delta),
+				    value_type (delta), "%g");
+      fprintf_filtered (stream, "delta %s", str.c_str());
+      if (!value_equal (delta, small))
+	{
+	  str = target_float_to_string (value_contents (small),
+					value_type (small), "%g");
+	  fprintf_filtered (stream, " <'small = %s>", str.c_str());
+	}
     }
 }
 
Index: binutils-gdb/gdb/ada-valprint.c
===================================================================
--- binutils-gdb.orig/gdb/ada-valprint.c
+++ binutils-gdb/gdb/ada-valprint.c
@@ -31,6 +31,7 @@ 
 #include "c-lang.h"
 #include "infcall.h"
 #include "objfiles.h"
+#include "target-float.h"
 
 static int print_field_values (struct type *, const gdb_byte *,
 			       int,
@@ -796,10 +797,15 @@  ada_val_print_num (struct type *type, co
 {
   if (ada_is_fixed_point_type (type))
     {
-      LONGEST v = unpack_long (type, valaddr + offset_aligned);
-
-      fprintf_filtered (stream, TYPE_LENGTH (type) < 4 ? "%.11g" : "%.17g",
-			(double) ada_fixed_to_float (type, v));
+      struct value *scale = ada_scaling_factor (type);
+      struct value *v = value_from_contents (type, valaddr + offset_aligned);
+      v = value_cast (value_type (scale), v);
+      v = value_binop (v, scale, BINOP_MUL);
+
+      const char *fmt = TYPE_LENGTH (type) < 4 ? "%.11g" : "%.17g";
+      std::string str
+	= target_float_to_string (value_contents (v), value_type (v), fmt);
+      fputs_filtered (str.c_str (), stream);
       return;
     }
   else if (TYPE_CODE (type) == TYPE_CODE_RANGE)