RE: [PATCH] The 2nd patch from the fixed-point branch

["Fu, Chao-Ying" <fu at mips dot com>]

Richard Sandiford wrote:

> > +static int check_real_for_fixed_mode (REAL_VALUE_TYPE *, 
> unsigned int);
> > +static int get_fixed_sign_bit (double_int, int);
> > +static bool do_fixed_add (FIXED_VALUE_TYPE *, const 
> FIXED_VALUE_TYPE *,
> > +			  const FIXED_VALUE_TYPE *, int, int);
> > +static bool do_fixed_multiply (FIXED_VALUE_TYPE *, const 
> FIXED_VALUE_TYPE *,
> > +			       const FIXED_VALUE_TYPE *, int);
> > +static bool do_fixed_divide (FIXED_VALUE_TYPE *, const 
> FIXED_VALUE_TYPE *,
> > +			     const FIXED_VALUE_TYPE *, int);
> > +static bool do_fixed_shift (FIXED_VALUE_TYPE *, const 
> FIXED_VALUE_TYPE *,
> > +			    const FIXED_VALUE_TYPE *, int, int);
> > +static bool do_fixed_neg (FIXED_VALUE_TYPE *, const 
> FIXED_VALUE_TYPE *, int);
> > +static bool fixed_saturate1 (unsigned int, double_int, 
> double_int *, int);
> > +static bool fixed_saturate2 (unsigned int, double_int, double_int,
> > +			     double_int *, int);
> 
> New style is not to have these forward declarations unless they are
> really needed (due to mutual recursion, etc.)

  Yes.  These declarations are deleted.

> 
> > +/* Compare two fixed objects for bitwise identity.  */
> > +
> > +bool
> > +fixed_identical (const FIXED_VALUE_TYPE *a, const 
> FIXED_VALUE_TYPE *b)
> > +{
> > +  return a->mode == b->mode && a->data.high == b->data.high
> > +	 && a->data.low == b->data.low;
> 
> Formatting nit, should be:
> 
>   return (a->mode == b->mode
> 	  && a->data.high == b->data.high
> 	  && a->data.low == b->data.low);

  Yes.

> 
> > +
> > +/* Check REAL_VALUE against the range of the fixed-point mode.
> > +   Return 0, if it is within the range.
> > +          1, if it is less than the minimum.
> > +          2, if it is greater than the maximum, but not equal to
> > +	     the maximum + the epsilon.
> > +          3, if it is equal to the maximum + the epsilon.  */
> 
> Would the code be clearer if this were an enum?  Or is this mirroring
> some well-known library function?

  Yes.  I created an enum for the returned values.

> 
> > +int
> > +check_real_for_fixed_mode (REAL_VALUE_TYPE *real_value, 
> unsigned int mode)
> 
> Why is MODE not an enum machine_mode?  (I'm sure there's a 
> good reason,
> I just wasn't sure what it was.)

  I just changed to enum machine_mode.

> 
> > +{
> > +  REAL_VALUE_TYPE max_value, min_value, epsilon_value;
> > +  char max_string[20], min_string[20], epsilon_string[20];
> > +
> > +  sprintf (max_string, "0x1.0p%d", GET_MODE_IBIT (mode));
> > +  sprintf (epsilon_string, "0x1.0p-%d", GET_MODE_FBIT (mode));
> > +  real_from_string (&max_value, max_string);
> > +  real_from_string (&epsilon_value, epsilon_string);
> 
> Couldn't you use real_2expN here instead?  Other cases later in file.

  Yes.  I replaced them with real_2expN.

> 
> > +void fixed_to_decimal (char *str, const FIXED_VALUE_TYPE *f_orig,
> > +		       size_t buf_size)
> 
> Formatting; function name should start a new line.

  Yes.

> 
> > +/* If SATP, saturate A to the maximum or the minimum, and 
> save to *F based on
> > +   the machine mode MODE.
> > +   This function assumes the width of double_int is 
> greater than the width
> > +   of the fixed-point value at the fixed-point mode.
> > +   Return true, if !SATP and overflow.  */
> > +
> > +static bool
> > +fixed_saturate1 (unsigned int mode, double_int a, 
> double_int *f, int satp)
> 
> Maybe satp should be a bool too?  I don't quite understand what you
> mean by "the fixed-point value at the fixed-point mode".

  SATP is a bool now.
  I try to say that the width is the sum of a possible sign bit, possible ibits 
and fbits => without any padding bits.

> 
> It might be clearer if you add "Do not modify *F otherwise." after the
> first sentence.  Same comments apply to fixed_saturate2.

  Yes.

> 
> > +/* Return the sign bit based on I_F_BITS.  */
> > +
> > +inline int get_fixed_sign_bit (double_int a, int i_f_bits)
> 
> Must be "static inline".  Formatting: function on new line.
> 
  Yes.

> > +      if (((!subtract_p)
> 
> Redundant brackets.

  Yes.

> 
> > +	   && get_fixed_sign_bit (a->data, i_f_bits)
> > +	      == get_fixed_sign_bit (b->data, i_f_bits)
> 
> Multiline conditions should be bracketed (so that emacs will indent
> the "==" correctly).

  Yes.

> 
> > +/* Calculate F = A << B if LEFT_P.  Otherwies, F = A >> B.
> > +   If SATP, saturate the result to the max or the min.
> > +   Return true, if !SATP and overflow.  */
> 
> Mixture of naming styles: LEFT_P vs. SATP.  FWIW, I think LEFT_P
> is the usual convention in gcc.  (Same thing with SUBTRACT_P earlier.)

  I changed SATP to SAT_P.

> 
> > +/* Perform the binary or unary operation described by CODE.
> > +   For a unary operation, leave OP1 NULL.
> > +   Return true, if !SATP and overflow.  */
> > +
> > +bool
> > +fixed_arithmetic (FIXED_VALUE_TYPE *f, int icode, const 
> FIXED_VALUE_TYPE *op0,
> > +		  const FIXED_VALUE_TYPE *op1, int satp)
> 
> 
> Am I right in thinking that OP0 and OP1 must have the same mode?
> If so, it might be worth a comment and a gcc_assert().

  Yes.

> The reason I ask is that...
> 
> > +/* Compare fixed-point values by tree_code.  */
> > +
> > +bool
> > +fixed_compare (int icode, const FIXED_VALUE_TYPE *op0,
> > +	       const FIXED_VALUE_TYPE *op1)
> > +{
> > +  enum tree_code code = icode;
> > +
> > +  switch (code)
> > +    {
> > +    case NE_EXPR:
> > +      return op0->mode != op1->mode
> > +	     || !double_int_equal_p (op0->data, op1->data);
> 
> ...I was somewhat surprised that fixed_arithmentic appeared to need
> operands of the same mode, but fixed_compare didn't.  It also wasn't
> immediately obvious that values of different modes were treated as
> unequal, even if they have the same conceptual value.  Might be worth
> adding a comment.

  "fixed_compare" must have the same mode as well.  I changed the comment
and the code to have gcc_assert.

> 
> > +/* Constant fixed-point values 0 and 1.  */
> > +extern FIXED_VALUE_TYPE fconst0[];
> > +extern FIXED_VALUE_TYPE fconst1[];
> 
> What are the indexes?

  Ok.  I created two defines and two new macros to access them.

#define MAX_FCONST0     18      /* For storing 18 fixed-point zeros per
                                   fract, ufract, accum, and uaccum modes .  */
#define MAX_FCONST1     8       /* For storing 8 fixed-point ones per accum
                                   and uaccum modes.  */
/* Constant fixed-point values 0 and 1.  */
extern FIXED_VALUE_TYPE fconst0[MAX_FCONST0];
extern FIXED_VALUE_TYPE fconst1[MAX_FCONST1];

/* Macros to access fconst0 and fconst1 via machine modes.  */
#define FCONST0(mode)   fconst0[mode - QQmode]
#define FCONST1(mode)   fconst1[mode - HAmode]


> 
> > +/* Determine whether a fixed-point value X is negative.  */
> > +#define FIXED_VALUE_NEGATIVE(x)          fixed_isneg (&(x))
> 
> Odd spacing (in context).

  Yes.

  The new patch is attached.  Thanks a lot!

Regards,
Chao-ying

2007-06-25  Chao-ying Fu  <fu@mips.com>

	* gengtype.c (main): Handle FIXED_VALUE_TYPE type as scalar typedef.
	* double-int.c (double_int_scmp): Use casts of unsigned HOST_WIDE_INT
	to compare a.low and b.low.
	* fixed-value.c: New file.
	* fixed-value.h: New file.
	* Makefile.in (OBJS-common): Add fixed-value.o.
	(fixed-value.o): New rule.
	(GTFILES): Add fixed-value.h.

Attachment: fixed-value.diff
Description: fixed-value.diff