RE: [patch] Fixed-point patch 8/10

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

Richard Sandiford wrote:
> 
> (BTW, can you generate patches with -d?  It makes them easier 
> to review.)

  Ok.  I set QUILT_DIFF_OPTS to "-dp" to get the diff file.

>
> "Fu, Chao-Ying" <fu@mips.com> writes:
> > +  else if (ALL_FIXED_POINT_MODE_P (GET_MODE (cmp_operands[0])))
> > +    {
> > +      enum rtx_code cmp_code;
> > +      cmp_code = *code;
> > +      *code = NE;
> > +      *op0 = gen_rtx_REG (CCDSPmode, CCDSP_CC_REGNUM);
> > +      *op1 = const0_rtx;
> > +      mips_emit_binary (cmp_code, *op0, cmp_operands[0], 
> cmp_operands[1]);
> > +    }
> 
> Simpler as:
> 
>    {
>      *op0 = gen_rtx_REG (CCDSPmode, CCDSP_CC_REGNUM);
>      mips_emit_binary (cmp_code, *op0, cmp_operands[0], 
> cmp_operands[1]);
>      *code = NE;
>      *op1 = const0_rtx;
>    }

  Yes with your change of cmp_code to *code.

> 
> (The FP case is different because it needs to modify cmp_code.)
> 
> >    else
> >      {
> >        enum rtx_code cmp_code;
> > @@ -4308,7 +4321,9 @@
> >       stack argument is passed in the last byte of the 
> stack slot.  */
> >    if (type != 0
> >        ? INTEGRAL_TYPE_P (type) || POINTER_TYPE_P (type)
> > -      : GET_MODE_CLASS (mode) == MODE_INT)
> > +	|| FIXED_POINT_TYPE_P (type)
> > +      : GET_MODE_CLASS (mode) == MODE_INT
> > +	|| ALL_SCALAR_FIXED_POINT_MODE_P (mode))
> >      return false;
> >  
> 
> Formatting should be:
> 
> if (type != 0
>     ? (INTEGRAL_TYPE_P (type)
>        || POINTER_TYPE_P (type)
>        || FIXED_POINT_TYPE_P (type))
>     : (GET_MODE_CLASS (mode) == MODE_INT
>        || ALL_SCALAR_FIXED_POINT_MODE_P (mode)))
> 
> (the "emacs brackets").  

  Yes.  I add the brackets.

> 
> > +/* Target hook for scalar_mode_supported_p.  */
> 
> Minor nit, but I'd prefer "Implement TARGET_SCALAR_MODE_SUPPORTED_P."
> It's then easier to tell where one should look for documentation.

  Yes.  The comment is changed.

> 
> > +
> > +static bool
> > +mips_scalar_mode_supported_p (enum machine_mode mode)
> > +{
> > +  if (ALL_FIXED_POINT_MODE_P (mode)
> > +      && GET_MODE_PRECISION (mode) <=  2 * BITS_PER_WORD)
> 
> Stray space.

  Yes.  It is deleted.

> 
> > +/* FIXME.  LONG_LONG_ACCUM_TYPE_SIZE should be 128 bits, but GCC
> > +   doesn't support 128-bit integers for MIPS32 currently.  */
> > +#define LONG_LONG_ACCUM_TYPE_SIZE (TARGET_LONG64 ? 128 : 64)
> 
> Gross as it is, we actually "support" -mlong64 for MIPS32.
> (As in "-mabi=eabi -mgp32 -mlong64".)  The FIXME implies that
> the correct value would be 128 unconditionally, so if 32-bitness
> is the problem, shouldn't we be checking TARGET_64BIT instead?

  Yes.  I change it to TARGET_64BIT.

> 
> > Index: gcc4x/gcc/gcc/config/mips/mips-fixed.md
> > ===================================================================
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ gcc4x/gcc/gcc/config/mips/mips-fixed.md	2007-07-30 
> 11:05:08.000000000 -0700
> > @@ -0,0 +1,143 @@
> > +; This file contains MIPS instructions that support 
> fixed-point operations.
> 
> Very minor nit, but mips.md uses ";;" for comments in column 0,
> except for a few cases that have slipped by.  Let's 
> standardise on that.

  Yes. I use ";;" for comments in mips-fixed.md.

> 
> > +; In GPR templates, a string like "<dd>subu" will expand 
> to "subu" in the
> > +; 32-bit version and "dsubu" in the 64-bit version.
> > +(define_mode_attr dd [(QQ "") (HQ "") (SQ "") (DQ "d")
> > +		      (UQQ "") (UHQ "") (USQ "") (UDQ "d")
> > +		      (HA "") (SA "") (DA "d")
> > +		      (UHA "") (USA "") (UDA "d")])
> 
> FWIW, I think it'd be OK to add this to the "d" attribute in mips.md.

  Yes.  I add this to the "d" attribute.

> 
> > +; The integer mode has the same size as the fixed-point mode.
> > +(define_mode_attr imode [(QQ "QI") (HQ "HI") (SQ "SI") (DQ "DI")
> > +			 (UQQ "QI") (UHQ "HI") (USQ "SI") (UDQ "DI")
> > +			 (HA "HI") (SA "SI") (DA "DI")
> > +			 (UHA "HI") (USA "SI") (UDA "DI")
> > +			 (V4UQQ "SI") (V2UHQ "SI") (V2UHA "SI")
> > +			 (V2HQ "SI") (V2HA "SI")])
> 
> This too could probably go in mips.md, as it might be useful
> for other modes in future.  I think "IMODE" would be better
> than "imode" for consistency with the automatic "MODE" and
> "mode" attributes (and for consistency with "UNITMODE" in mips.md).

  Yes.  I move it to mips.md and change the name to IMODE.

> 
> Overall, this looks very clean, thanks.
> 

  Thanks a lot!  The new diff file is attached.

Regards,
Chao-ying

Attachment: mips.diff
Description: mips.diff