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

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

Hi,

  Any update for this patch?  Thanks!
(I have the 3rd patch almost ready to send out.)

Regards,
Chao-ying

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org
> [mailto:gcc-patches-owner@gcc.gnu.org]On Behalf Of Fu, Chao-Ying
> Sent: Tuesday, June 26, 2007 6:24 PM
> To: Richard Sandiford
> Cc: gcc-patches@gcc.gnu.org; Mark Mitchell; Thekkath, 
> Radhika; Stephens,
> Nigel
> Subject: RE: [PATCH] The 2nd patch from the fixed-point branch
> 
> 
> 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.
>