This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Intrinsics for PREFETCHW


> 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.
Sorry, what's wrong here?

> 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.
Sure!

> +DEF_FUNCTION_TYPE (UCHAR, UCHAR, UINT, UINT, PINT)
> +DEF_FUNCTION_TYPE (UCHAR, UCHAR, ULONGLONG, ULONGLONG, PINT)
Whoops, removed!

> +  /* RDSEED instructions. */
>
> Two spaces after the dot.
Fixed!

> +  IX86_BUILTIN_RDSEED16,
> +  IX86_BUILTIN_RDSEED32,
> +  IX86_BUILTIN_RDSEED64,
>
> Please name this IX86_BUILTIN_RDSEED{16,32,64}_STEP.
Fixed!

> +  /* 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
Fixed!

> +    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.
Thanks! Fixed. We need to return success/failure of rdseed execution.
It set by CF.

> +      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;
Thanks! I added this (slightly fixed).

>
> +
> +  UNSPEC_RDSEED
>
> Needs to be volatile. Please also add comment.
Done.

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

>
> +#if !defined _X86INTRIN_H_INCLUDED && !defined _IMMINTRIN_H_INCLUDED
>
> No need for immintrin.h check
Done.

Thanks fr review!
Here is updated patch. Tests passing:
ChangeLog entry:
2012-07-25  Kirill Yukhin  <kirill.yukhin@intel.com>
            Michael Zolotukhin  <michael.v.zolotukhin@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>_1): New.
        * config/i386/i386.opt (mrdseed): New.
        * config/i386/x86intrin.h: Include rdseedintrin.h.

testsuite/ChangeLog unchanged.

Is it OK?

Thanks, K

Attachment: bdw-rdseed-2.gcc.patch
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]