Re: [PATCH,rs6000] Add built-in support for new Power9 darn (deliver a random number) instruction

[Bill Schmidt <wschmidt@linux.vnet.ibm.com>]

On Mon, 2016-05-09 at 08:58 -0500, Segher Boessenkool wrote:
> Hi Kelvin,
> 
> On Thu, May 05, 2016 at 10:26:01AM -0600, Kelvin Nilsen wrote:
> > 	(UNSPEC_DARN_32): New usnpec constant.
> 
> Typo.
> 
> > 	("darn_32"): New instruction.
> 
> We don't normally use quotes for insn names.
> 
> > 	(rs6000_builtin_mask_calculate): Add in the RS6000_BTM_MODULO and
> > 	RS6000_BTM_64BIT flags to the returned mask, depending on
> > 	configuration. 
> 
> Trailing space (many, in this changelog).
> 
> > --- gcc/config/rs6000/altivec.h	(revision 235884)
> > +++ gcc/config/rs6000/altivec.h	(working copy)
> > @@ -382,6 +382,11 @@
> >  #define vec_vsubuqm __builtin_vec_vsubuqm
> >  #define vec_vupkhsw __builtin_vec_vupkhsw
> >  #define vec_vupklsw __builtin_vec_vupklsw
> > +
> > +/* Non-Vector additions added in ISA 3.0. */
> > +#define darn __builtin_darn
> > +#define darn_32 __builtin_darn_32
> > +#define darn_raw __builtin_darn_raw
> >  #endif
> 
> Do we really want to #define short words like "darn"?  If this is already
> set in stone, so be it.

I don't think we do, and in any case altivec.h would not be the place to
do it.  darn is not a vector instruction.

For these, just having __builtin_darn* be the available interfaces will
be fine.

My two cents,
Bill

> 
> > +(define_insn "darn_32"
> > +  [(set (match_operand:SI 0 "register_operand" "")
> 
> The constraint should be "r" I suppose?
> 
> > +        (unspec:SI [(const_int 0)] UNSPEC_DARN_32))]
> > +  "TARGET_MODULO"
> > +  {
> > +     return "darn %0,0";
> > +  }
> > +  [(set_attr "type" "add")  
> 
> Trailing spaces.  "add" isn't the correct type; use "integer" if there
> is no better type.
> 
> > +   (set_attr "length" "4")])
> 
> That is the default, no need to mention it.  Most insns are implicitly
> length 4.
> 
> > +/* Miscellaneous builtins for instructions added in ISA 3.0.  These
> > +   instructions don't require either the DFP or VSX options, just the basic 
> 
> Trailing space.
> 
> > @@ -3634,6 +3639,8 @@ rs6000_builtin_mask_calculate (void)
> >  	  | ((rs6000_cpu == PROCESSOR_CELL) ? RS6000_BTM_CELL      : 0)
> >  	  | ((TARGET_P8_VECTOR)		    ? RS6000_BTM_P8_VECTOR : 0)
> >  	  | ((TARGET_P9_VECTOR)		    ? RS6000_BTM_P9_VECTOR : 0)
> > +	  | ((TARGET_MODULO)		    ? RS6000_BTM_MODULO    : 0)
> > +	  | ((TARGET_64BIT)		    ? RS6000_BTM_64BIT    : 0)
> 
> Missing space?
> 
> > +  /* RS6000_BTC_SPECIAL represents no-operand operators.  */
> >    gcc_assert (attr == RS6000_BTC_UNARY
> >  	      || attr == RS6000_BTC_BINARY
> > -	      || attr == RS6000_BTC_TERNARY);
> > -
> > +	      || attr == RS6000_BTC_TERNARY
> > +	      || attr == RS6000_BTC_SPECIAL);
> > +  
> 
> Why SPECIAL and not NULLARY or such?
> 
> > +      if (rs6000_overloaded_builtin_p (d->code))
> > +	{
> > +	  if (! (type = opaque_ftype_opaque))
> > +	    type = opaque_ftype_opaque
> > +	      = build_function_type_list (opaque_V4SI_type_node,
> > +					  NULL_TREE);
> > +	}
> 
> Eww.
> 
>   if (!opaque_ftype_opaque)
>     opaque_ftype_opaque = build_function_type_list (...);
>   type = opaque_ftype_opaque;
> 
> > +	  enum insn_code icode = d->icode;
> > +	  if (d->name == 0)
> > +	    {
> > +	      if (TARGET_DEBUG_BUILTIN)
> > +		fprintf (stderr, "rs6000_builtin, bdesc_0arg[%ld] no name\n",
> > +			 (long unsigned)i);
> 
> unsigned is %u, not %d.  Space after cast.
> 
> Cheers,
> 
> 
> Segher
>