[rfc] Eliminate duplicated logic with decimal operations

[rfc] Eliminate duplicated logic with decimal operations

[Ulrich Weigand]

Hello,

I've noticed that during decimal floating point operations, there seems
to be some duplicated decisions regarding the result type.

In particular, decimal_binop provides a LEN_RESULT *output* parameter
that gets set to the length of the result type (which is always the
larger of the two input types).

However, the sole caller of the routine, value_binop, in fact simply
*ignores* that returned length value.  Instead, it uses a duplicated
logic to determine the full result type as the larger of the two
(promoted, if necessary) input types.

Subsequent code then simply assumes the value provided by decimal_binop
matches the type determined by value_binop.  This happens to be the
case today because the two instances of duplicated logic always come
to the same results.

It seems to me that it would be simpler to avoid that duplication.
The patch below only computes the result type in value_binop, and
passes the resulting length as *input* to decimal_binop, which will
then ensure it returns a value of that length.

This makes the rest of the promote_decimal routine superfluous --
its only effect are two extra conversions, which seem unnecessary:

  1. input (input_len) --> decNumber
  2. decNumber --> temp (output_len)
  3. temp (output_len) --> decNumber

The patch below removes steps 2 and 3 and only keeps the first
conversion to decNumber.  This applies to both decimal_binop and 
decimal_compare.

Thiago, any thoughts on this?  Do you agree that those conversions
are unnecessary?

Tested on powerpc64-linux with no regressions.

Bye,
Ulrich


ChangeLog:

	* dfp.h (decimal_binop): Convert LEN_RESULT to input parameter.
	* dfp.c (promote_decimal): Remove.
	(decimal_binop): Convert LEN_RESULT to input parameter.
	Remove call to decimal_binop.
	(decimal_compare): Remove call to decimal_binop.

	* valarith.c (value_binop): Pass desired result type length
	to decimal_binop.


Index: gdb-head/gdb/dfp.c
===================================================================
--- gdb-head.orig/gdb/dfp.c
+++ gdb-head/gdb/dfp.c
@@ -255,38 +255,11 @@ decimal_to_doublest (const gdb_byte *fro
   return strtod (buffer, NULL);
 }
 
-/* Check if operands have the same size and convert them to the
-   biggest of the two if necessary.  */
-static int
-promote_decimal (gdb_byte *x, int len_x, gdb_byte *y, int len_y)
-{
-  int len_result;
-  decNumber number;
-
-  if (len_x < len_y)
-    {
-      decimal_to_number (x, len_x, &number);
-      decimal_from_number (&number, x, len_y);
-      len_result = len_y;
-    }
-  else if (len_x > len_y)
-    {
-      decimal_to_number (y, len_y, &number);
-      decimal_from_number (&number, y, len_x);
-      len_result = len_x;
-    }
-  else
-    len_result = len_x;
-
-  return len_result;
-}
-
-/* Perform operation OP with operands X and Y and store value in RESULT.
-   If LEN_X and LEN_Y are not equal, RESULT will have the size of the biggest
-   of the two, and LEN_RESULT will be set accordingly.  */
+/* Perform operation OP with operands X and Y with sizes LEN_X and LEN_Y
+   and store value in RESULT with size LEN_RESULT.  */
 void
 decimal_binop (enum exp_opcode op, const gdb_byte *x, int len_x,
-	       const gdb_byte *y, int len_y, gdb_byte *result, int *len_result)
+	       const gdb_byte *y, int len_y, gdb_byte *result, int len_result)
 {
   decContext set;
   decNumber number1, number2, number3;
@@ -295,14 +268,10 @@ decimal_binop (enum exp_opcode op, const
   match_endianness (x, len_x, dec1);
   match_endianness (y, len_y, dec2);
 
-  *len_result = promote_decimal (dec1, len_x, dec2, len_y);
+  decimal_to_number (dec1, len_x, &number1);
+  decimal_to_number (dec2, len_y, &number2);
 
-  /* Both operands are of size *len_result from now on.  */
-
-  decimal_to_number (dec1, *len_result, &number1);
-  decimal_to_number (dec2, *len_result, &number2);
-
-  set_decnumber_context (&set, *len_result);
+  set_decnumber_context (&set, len_result);
 
   switch (op)
     {
@@ -330,9 +299,9 @@ decimal_binop (enum exp_opcode op, const
   /* Check for errors in the DFP operation.  */
   decimal_check_errors (&set);
 
-  decimal_from_number (&number3, dec3, *len_result);
+  decimal_from_number (&number3, dec3, len_result);
 
-  match_endianness (dec3, *len_result, result);
+  match_endianness (dec3, len_result, result);
 }
 
 /* Returns true if X (which is LEN bytes wide) is the number zero.  */
@@ -362,11 +331,11 @@ decimal_compare (const gdb_byte *x, int 
   match_endianness (x, len_x, dec1);
   match_endianness (y, len_y, dec2);
 
-  len_result = promote_decimal (dec1, len_x, dec2, len_y);
-
-  decimal_to_number (dec1, len_result, &number1);
-  decimal_to_number (dec2, len_result, &number2);
+  decimal_to_number (dec1, len_x, &number1);
+  decimal_to_number (dec2, len_y, &number2);
 
+  /* Perform the comparison in the larger of the two sizes.  */
+  len_result = len_x > len_y ? len_x : len_y;
   set_decnumber_context (&set, len_result);
 
   decNumberCompare (&result, &number1, &number2, &set);
Index: gdb-head/gdb/dfp.h
===================================================================
--- gdb-head.orig/gdb/dfp.h
+++ gdb-head/gdb/dfp.h
@@ -35,7 +35,7 @@ extern void decimal_from_integral (struc
 extern void decimal_from_floating (struct value *from, gdb_byte *to, int len);
 extern DOUBLEST decimal_to_doublest (const gdb_byte *from, int len);
 extern void decimal_binop (enum exp_opcode, const gdb_byte *, int,
-			   const gdb_byte *, int, gdb_byte *, int *);
+			   const gdb_byte *, int, gdb_byte *, int);
 extern int decimal_is_zero (const gdb_byte *x, int len);
 extern int decimal_compare (const gdb_byte *x, int len_x, const gdb_byte *y, int len_y);
 extern void decimal_convert (const gdb_byte *from, int len_from, gdb_byte *to,
Index: gdb-head/gdb/valarith.c
===================================================================
--- gdb-head.orig/gdb/valarith.c
+++ gdb-head/gdb/valarith.c
@@ -887,6 +887,19 @@ value_binop (struct value *arg1, struct 
       gdb_byte v1[16], v2[16];
       gdb_byte v[16];
 
+      /* If only one type is decimal float, use its type.
+	 Otherwise use the bigger type.  */
+      if (TYPE_CODE (type1) != TYPE_CODE_DECFLOAT)
+	result_type = type2;
+      else if (TYPE_CODE (type2) != TYPE_CODE_DECFLOAT)
+	result_type = type1;
+      else if (TYPE_LENGTH (type2) > TYPE_LENGTH (type1))
+	result_type = type2;
+      else
+	result_type = type1;
+
+      len_v = TYPE_LENGTH (result_type);
+
       value_args_as_decimal (arg1, arg2, v1, &len_v1, v2, &len_v2);
 
       switch (op)
@@ -896,24 +909,13 @@ value_binop (struct value *arg1, struct 
 	case BINOP_MUL:
 	case BINOP_DIV:
 	case BINOP_EXP:
-	  decimal_binop (op, v1, len_v1, v2, len_v2, v, &len_v);
+	  decimal_binop (op, v1, len_v1, v2, len_v2, v, len_v);
 	  break;
 
 	default:
 	  error (_("Operation not valid for decimal floating point number."));
 	}
 
-      /* If only one type is decimal float, use its type.
-	 Otherwise use the bigger type.  */
-      if (TYPE_CODE (type1) != TYPE_CODE_DECFLOAT)
-	result_type = type2;
-      else if (TYPE_CODE (type2) != TYPE_CODE_DECFLOAT)
-	result_type = type1;
-      else if (TYPE_LENGTH (type2) > TYPE_LENGTH (type1))
-	result_type = type2;
-      else
-	result_type = type1;
-
       val = value_from_decfloat (result_type, v);
     }
   else if (TYPE_CODE (type1) == TYPE_CODE_FLT
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Eliminate duplicated logic with decimal operations

[Thiago Jung Bauermann]

Hi Uli,

Em Qui, 2009-05-14 às 13:32 +0200, Ulrich Weigand escreveu:
> The patch below removes steps 2 and 3 and only keeps the first
> conversion to decNumber.  This applies to both decimal_binop and 
> decimal_compare.
> 
> Thiago, any thoughts on this?  Do you agree that those conversions
> are unnecessary?

Thanks for this improvement! I agree with you, those are silly
conversions.

> Tested on powerpc64-linux with no regressions.

Silly question: you ran the testsuite with a DFP-capable compiler
right? :-)

dfp-exprs.exp has promotion tests, so your patch is certainly good.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [rfc] Eliminate duplicated logic with decimal operations

[Ulrich Weigand]

Hi Thiago,

> Em Qui, 2009-05-14 às 13:32 +0200, Ulrich Weigand escreveu:
> > The patch below removes steps 2 and 3 and only keeps the first
> > conversion to decNumber.  This applies to both decimal_binop and 
> > decimal_compare.
> > 
> > Thiago, any thoughts on this?  Do you agree that those conversions
> > are unnecessary?
> 
> Thanks for this improvement! I agree with you, those are silly
> conversions.

Thanks for your review.  I've checked this patch in now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com