Message ID | CAFULd4aibPcJihvksZJrdMgRD+m+8EUYHcvKru7kJVNcUyi+-A@mail.gmail.com |
---|---|
State | New |
Headers | show |
> Please see attached patch that does all this with minimum surgery and > also sets missing SSE prefetch for x86_32 with -mprfchw Thanks, applied! > +++ b/gcc/config/i386/prfchwintrin.h > > +#if !defined _X86INTRIN_H_INCLUDED && !defined _MM3DNOW_H_INCLUDED > +# error "Never use <prfchwintrin.h> directly; include <x86intrin.h>\ > +or <mm3dnow.h> instead." > +#endif > > No need to split the line. Fixed. > --- a/gcc/config/i386/x86intrin.h > +++ b/gcc/config/i386/x86intrin.h > @@ -61,6 +61,10 @@ > /* For including AVX instructions */ > #include <immintrin.h> > > +#if defined (__PRFCHW__) || defined (__3dNOW__) > +#include <prfchwintrin.h> > +#endif > > Not needed. This header is already included through mm3dnow.h > inclusion and directly below Fixed. Updated Changelog: 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_PRFCHW_SET): New. (OPTION_MASK_ISA_PRFCHW_UNSET): Likewise. (ix86_handle_option): Handle mprfchw option. * config.gcc (i[34567]86-*-*): Add prfchwintrin.h. (x86_64-*-*): Likewise. * config/i386/prfchwintrin.h: New header. * config/i386/cpuid.h (bit_PRFCHW): New. (bit_BMI): Formatting fix. (bit_HLE): Likewise. (bit_RTM): Likewise. * config/i386/driver-i386.c (host_detect_local_cpu): Detect PREFETCHW support. * config/i386/i386-c.c: Define __PRFCHW__ if needed. * config/i386/i386.c (ix86_target_string): Define -mprfchw option. Formatting fixes. (PTA_HLE): Formatting fix. (PTA_PRFCHW): New. (ix86_option_override_internal): Handle new option. (ix86_valid_target_attribute_inner_p): Add OPT_mprfchw. * config/i386/i386.h (TARGET_PRFCHW): New. * config/i386/i386.md (prefetch): Enable for TARGET_PRFCHW. * config/i386/i386.opt (mprfchw): New. * config/i386/mm3dnow.h: Move _m_prefetchw from here to prfchwintrin.h. * config/i386/x86intrin.h: Include prfchwintrin.h. testsuite/Changelog entry wasn't changed. Bootstrap is passing. New & updated tests passing. Is it OK for trunk? Thanks, K
On Wed, Jul 25, 2012 at 1:51 PM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote: >> Please see attached patch that does all this with minimum surgery and >> also sets missing SSE prefetch for x86_32 with -mprfchw > Thanks, applied! > >> +++ b/gcc/config/i386/prfchwintrin.h >> >> +#if !defined _X86INTRIN_H_INCLUDED && !defined _MM3DNOW_H_INCLUDED >> +# error "Never use <prfchwintrin.h> directly; include <x86intrin.h>\ >> +or <mm3dnow.h> instead." >> +#endif >> >> No need to split the line. > Fixed. > >> --- a/gcc/config/i386/x86intrin.h >> +++ b/gcc/config/i386/x86intrin.h >> @@ -61,6 +61,10 @@ >> /* For including AVX instructions */ >> #include <immintrin.h> >> >> +#if defined (__PRFCHW__) || defined (__3dNOW__) >> +#include <prfchwintrin.h> >> +#endif >> >> Not needed. This header is already included through mm3dnow.h >> inclusion and directly below > Fixed. > > Updated Changelog: > 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_PRFCHW_SET): New. > (OPTION_MASK_ISA_PRFCHW_UNSET): Likewise. > (ix86_handle_option): Handle mprfchw option. > * config.gcc (i[34567]86-*-*): Add prfchwintrin.h. > (x86_64-*-*): Likewise. > * config/i386/prfchwintrin.h: New header. > * config/i386/cpuid.h (bit_PRFCHW): New. > (bit_BMI): Formatting fix. > (bit_HLE): Likewise. > (bit_RTM): Likewise. > * config/i386/driver-i386.c (host_detect_local_cpu): Detect > PREFETCHW support. > * config/i386/i386-c.c: Define __PRFCHW__ if needed. > * config/i386/i386.c (ix86_target_string): Define > -mprfchw option. Formatting fixes. > (PTA_HLE): Formatting fix. > (PTA_PRFCHW): New. > (ix86_option_override_internal): Handle new option. > (ix86_valid_target_attribute_inner_p): Add OPT_mprfchw. > * config/i386/i386.h (TARGET_PRFCHW): New. > * config/i386/i386.md (prefetch): Enable for TARGET_PRFCHW. > * config/i386/i386.opt (mprfchw): New. > * config/i386/mm3dnow.h: Move _m_prefetchw from here to > prfchwintrin.h. > * config/i386/x86intrin.h: Include prfchwintrin.h. > > testsuite/Changelog entry wasn't changed. > > Bootstrap is passing. New & updated tests passing. > > Is it OK for trunk? OK. Thanks, Uros.
>> Is it OK for trunk? > > OK. Thanks! Checked in. http://gcc.gnu.org/viewcvs?view=revision&revision=189844 Next I think would be rdseed* insns. Thanks, K
Hi again, Here is second patch which adds support of rdseed[16|32|64] insn. 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>): New. * config/i386/i386.opt (mrdseed): New. * config/i386/x86intrin.h: Include rdseedintrin.h. tessuite/Changelog entry: 2012-07-24 Kirill Yukhin <kirill.yukhin@intel.com> Michael Zolotukhin <michael.v.zolotukhin@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. * 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. It was bootstrapped. New and updated tests passing. Is it OK for trunk? Thanks, K On Wed, Jul 25, 2012 at 5:03 PM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote: >>> Is it OK for trunk? >> >> OK. > > Thanks! > Checked in. > http://gcc.gnu.org/viewcvs?view=revision&revision=189844 > > Next I think would be rdseed* insns. > > Thanks, K
On Wed, Jul 25, 2012 at 4:07 PM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote: > Here is second patch which adds support of rdseed[16|32|64] insn. > > 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>): New. > * config/i386/i386.opt (mrdseed): New. > * config/i386/x86intrin.h: Include rdseedintrin.h. > > tessuite/Changelog entry: > 2012-07-24 Kirill Yukhin <kirill.yukhin@intel.com> > Michael Zolotukhin <michael.v.zolotukhin@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.
> 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
On Mon, Jul 30, 2012 at 2:05 PM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote: >> 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? Not here, but above "Ehm..." line you have: > * gcc.target/i386/sse-12.c: Add -mprfchw. You should add something else for rdseed. Uros.
--- a/gcc/config/i386/x86intrin.h +++ b/gcc/config/i386/x86intrin.h @@ -61,6 +61,10 @@ /* For including AVX instructions */ #include <immintrin.h> +#if defined (__PRFCHW__) || defined (__3dNOW__) +#include <prfchwintrin.h> +#endif Not needed. This header is already included through mm3dnow.h inclusion and directly below