This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH,rs6000] Add built-in support for new Power9 darn (deliver a random number) instruction
- From: Bill Schmidt <wschmidt at linux dot vnet dot ibm dot com>
- To: Segher Boessenkool <segher at kernel dot crashing dot org>
- Cc: Kelvin Nilsen <kdnilsen at linux dot vnet dot ibm dot com>, gcc-patches at gcc dot gnu dot org
- Date: Mon, 09 May 2016 12:35:56 -0500
- Subject: Re: [PATCH,rs6000] Add built-in support for new Power9 darn (deliver a random number) instruction
- Authentication-results: sourceware.org; auth=none
- References: <572B7419 dot 7030205 at linux dot vnet dot ibm dot com> <20160509135835 dot GB31139 at gate dot crashing dot org>
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
>