On Wed, Jul 25, 2012 at 4:07 PM, Kirill Yukhin <kirill.yuk...@gmail.com> wrote:

> Here is second patch which adds support of rdseed[16|32|64] insn.
>
> Changelog entry:
> 2012-07-25  Kirill Yukhin  <kirill.yuk...@intel.com>
>             Michael Zolotukhin  <michael.v.zolotuk...@intel.com>
>
>         * common/config/i386/i386-common.c (OPTION_MASK_ISA_RDSEED_SET): New.
>         (OPTION_MASK_ISA_RDSEED_UNSET): Likewise.
>         (ix86_handle_option): Handle mrdseed option.
>         * config.gcc (i[34567]86-*-*): Add rdseedintrin.h.
>         (x86_64-*-*): Likewise.
>         * config/i386/prfchwintrin.h: New header.
>         * config/i386/cpuid.h (bit_RDSEED): New.
>         * config/i386/driver-i386.c (host_detect_local_cpu): Detect
>         RDSEED support.
>         * config/i386/i386-c.c: Define __RDSEED__ if needed.
>         * config/i386/i386.c (ix86_target_string): Define
>         -mrdseed option.
>         (PTA_RDSEED): New.
>         (ix86_option_override_internal): Handle new option.
>         (ix86_valid_target_attribute_inner_p): Add OPT_mrdseed.
>         (ix86_builtins): Add enum entries for RDSEED* builtins.
>         (ix86_init_mmx_sse_builtins): Define new builtins.
>         (ix86_expand_builtin): Expand RDSEED* builtins.
>         * config/i386/i386.h (TARGET_RDSEED): New.
>         * config/i386/i386.md (rdseed<mode>): New.
>         * config/i386/i386.opt (mrdseed): New.
>         * config/i386/x86intrin.h: Include rdseedintrin.h.
>
> tessuite/Changelog entry:
> 2012-07-24  Kirill Yukhin  <kirill.yuk...@intel.com>
>             Michael Zolotukhin  <michael.v.zolotuk...@intel.com>
>
>         * gcc.target/i386/rdseed16-1.c: New.
>         * gcc.target/i386/rdseed32-1.c: Ditto
>         * gcc.target/i386/rdseed64-1.c: Ditto
>         * gcc.target/i386/sse-12.c: Add -mprfchw.

Ehm ...

>         * gcc.target/i386/sse-13.c: Ditto.
>         * gcc.target/i386/sse-14.c: Ditto.
>         * g++.dg/other/i386-2.C: Ditto.
>         * g++.dg/other/i386-3.C: Ditto.

I suggest you implement handling of this builtin in the same way
rdrand<mode>_1 is implemented. Please also keep names of builtins and
enums consistent with rdrand. Also, please put new defines just after
rdrand stuff, they somehow belongs together.

+DEF_FUNCTION_TYPE (UCHAR, UCHAR, UINT, UINT, PINT)
+DEF_FUNCTION_TYPE (UCHAR, UCHAR, ULONGLONG, ULONGLONG, PINT)

Not yet.

+  /* RDSEED instructions. */

Two spaces after the dot.

+  IX86_BUILTIN_RDSEED16,
+  IX86_BUILTIN_RDSEED32,
+  IX86_BUILTIN_RDSEED64,

Please name this IX86_BUILTIN_RDSEED{16,32,64}_STEP.

+  /* RDSEED */
+  def_builtin (OPTION_MASK_ISA_RDSEED, "__builtin_ia32_rdseed_hi",
+              INT_FTYPE_PUSHORT, IX86_BUILTIN_RDSEED16);
+  def_builtin (OPTION_MASK_ISA_RDSEED, "__builtin_ia32_rdseed_si",
+              INT_FTYPE_PUNSIGNED, IX86_BUILTIN_RDSEED32);
+  def_builtin (OPTION_MASK_ISA_RDSEED && OPTION_MASK_ISA_64BIT,
+              "__builtin_ia32_rdseed_di",
+              INT_FTYPE_PULONGLONG, IX86_BUILTIN_RDSEED64);

__builtin_ia32_rdseed{16,32,64}_step

+    case IX86_BUILTIN_RDSEED16:
+    case IX86_BUILTIN_RDSEED32:
+    case IX86_BUILTIN_RDSEED64:

Just copy from rdrand handling everything, up to:

      emit_move_insn (gen_rtx_MEM (mode0, op1), op0);

+      /* Generate random number and save it in OP0.  */

+      /* Store the result to sum.  */

+      /* Return current CF value.  */

No need for comments. BTW: You are probably returning a seed, not a
random value.

+      emit_insn (gen_rtx_SET (QImode, target,
+           gen_rtx_LTU (QImode, gen_rtx_REG (CCCmode, FLAGS_REG), 
const0_rtx)));

This is wrong. Try following (untested) code:

      op2 = gen_reg_rtx (QImode);

      pat = gen_rtx_LTU (QImode, gen_rtx_REG (CCCmode, FLAGS_REG),
                         const0_rtx);
      emit_insn (gen_rtx_SET (VOIDmode, op2, pat));

      if (target == 0)
        target = gen_reg_rtx (SImode);

      emit_insn (gen_zero_extendqisi2 (target, op0));
      return target;

+
+  UNSPEC_RDSEED

Needs to be volatile. Please also add comment.

+(define_insn "rdseed<mode>"
+  [(set (match_operand:SWI248 0 "register_operand" "=r")
+       (unspec:SWI248 [ (const_int 0) ] UNSPEC_RDSEED))]
+  "TARGET_RDSEED"
+  "rdseed\t%0"
+  [(set_attr "mode" "<MODE>")])

Wrong! Please copy pattern from "rdrand<mode>_1" (also, please name it
in the same way).

+#if !defined _X86INTRIN_H_INCLUDED && !defined _IMMINTRIN_H_INCLUDED

No need for immintrin.h check

Uros.

Reply via email to