This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [fixed-point] Patch to parse _Fract, _Accum, _Sat


Hello,

  Thanks for your review!

> > +  unsigned saturating_flag : 1;
>
> You probably have noticed that the flags are grouped in sets of 8 bits,
> and that there are four groups of 8 bits.  You've added another bit,
> and increased the size of tree_common by a full word.  Is that change
> on purpose?

  About saturating_flag, if I cannot add a new flag in either tree_common
or tree_type, I will need to use lang_flag_2  in c-tree.h
Ex:
#define C_TYPE_SATURATING(TYPE) TYPE_LANG_FLAG_2 (TYPE)

> > +  TI_SAT_SFRACT_TYPE,
> > +  TI_SAT_FRACT_TYPE,
> > +  TI_SAT_LFRACT_TYPE,
> > +  TI_SAT_LLFRACT_TYPE,
> > +  TI_SAT_USFRACT_TYPE,
> > +  TI_SAT_UFRACT_TYPE,
> > +  TI_SAT_ULFRACT_TYPE,
> > +  TI_SAT_ULLFRACT_TYPE,
>
> The usual practice in GCC is to use machine modes instead of C type
> names.  I'm not sure what that would exactly look like in this case,
> but could you have e.g. TI_SAT_UFRACTSI_TYPE instead of
> TI_SAT_ULFRACT_TYPE?

  These TI*s are for the standard named data types. Their machine modes
will be different on different backends  (like the "long" type ).
However, I can change the names for the nameless data types.
Ex:
TI_QQ_TYPE -> TI_FRACTQQ_TYPE
TI_UQQ_TYPE -> TI_UFRACTQQ_TYPE
...

> > +static tree
> > +make_or_reuse_fract_type (unsigned size, int unsignedp, int satp)
>
> New function, needs a comment before it.  Likewise for all other new
> functions.

  Yes.  I will add comments.

> > @@ -1523,7 +1532,12 @@
> >       _Decimal32
> >       _Decimal64
> >       _Decimal128
> > +     _Fract
> > +     _Accum
> >
> > +   type-qualifier:
> > +     _Sat
> > +
>
> You've put _Sat under GNU extensions.  I see that the dfp type specifiers
> are under that header too but I thought they come from some standard.
> Anyway, if _Sat comes from a standard, it deserves its own heading here
> (and perhaps you could point to the relevant standards in a comment
> everywhere else where you add things for fixed-point parsing??).

  _Fract, _Accum and _Sat are from this DTR.
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1021.pdf
I will add this link to the comment.

> > +  /* Fixed-point types. */
> > +  sat_short_fract_type_node = make_sat_signed_fract_type
(SHORT_FRACT_TYPE_SIZE);
> > +  sat_fract_type_node = make_sat_signed_fract_type (FRACT_TYPE_SIZE);
> > +  sat_long_fract_type_node = make_sat_signed_fract_type
(LONG_FRACT_TYPE_SIZE);
> > +  sat_long_long_fract_type_node = make_sat_signed_fract_type
(LONG_LONG_FRACT_TYPE
>
> Should this not be conditional on targetm.fixed_point_supported_p() ?

  Yes.  I will put the test.

  Thanks a lot!

Regards,
Chao-ying


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]