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: Segher Boessenkool <segher at kernel dot crashing dot org>
- To: Kelvin Nilsen <kdnilsen at linux dot vnet dot ibm dot com>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Mon, 9 May 2016 08:58:35 -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>
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.
> +(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