On Fri, Dec 18, 2015 at 8:15 AM, Kirill Yukhin <[email protected]> wrote:
> Hello,
> Patch in the bottom introduces support Intel PKRU instructions:
> rdpkru and wrpkru.
> It is pretty straight-forward, so I hope it is still suitable for v6.
>
> Names for new intrinsics will appear shortly in new revision of SDM.
>
> Bootstrapped & regtested.
>
> Is it ok for trunk?
The patch mostly looks OK, but md patterns are written in the wrong
way. Please see comments bellow.
> gcc/
> * common/config/i386/i386-common.c (OPTION_MASK_ISA_PKU_SET): New.
> (OPTION_MASK_ISA_PKU_UNSET): Ditto.
> (ix86_handle_option): Handle OPT_mpku.
> * config.gcc: Add pkuintrin.h to i[34567]86-*-* and x86_64-*-*
> targets.
> * config/i386/cpuid.h (host_detect_local_cpu): Detect PKU feature.
> * config/i386/i386-c.c (ix86_target_macros_internal): Handle PKU ISA
> flag.
> * config/i386/i386.c (ix86_target_string): Add "-mpku" to
> ix86_target_opts.
> (ix86_option_override_internal): Define PTA_PKU, mention new key
> in skylake-avx512. Handle new ISA bits.
> (ix86_valid_target_attribute_inner_p): Add "pku".
> (enum ix86_builtins): Add IX86_BUILTIN_RDPKRU and IX86_BUILTIN_WRPKRU.
> (builtin_description bdesc_special_args[]): Add new built-ins.
> * config/i386/i386.h (define TARGET_PKU): New.
> (define TARGET_PKU_P): Ditto.
> * config/i386/i386.md (define_c_enum "unspec"): Add UNSPEC_PKU.
> (define_c_enum "unspecv"): Add UNSPECV_PKU.
> (define_expand "rdpkru"): New.
> (define_insn "rdpkru_2"): Ditto.
> (define_expand "wrpkru"): Ditto.
> (define_insn "wrpkru_2"): Ditto.
> * config/i386/i386.opt (mpku): Ditto.
> * config/i386/pkuintrin.h: New file.
> * config/i386/x86intrin.h: Include pkuintrin.h
> * doc/extend.texi: Describe new built-ins.
> * doc/invoke.texi: Describe new switches.
>
> gcc/testsuite/
> * g++.dg/other/i386-2.C: Add -mpku.
> * g++.dg/other/i386-3.C: Ditto.
> * gcc.target/i386/rdpku-1.c: New test.
> * gcc.target/i386/sse-12.c: Add -mpku.
> * gcc.target/i386/sse-13.c: Ditto..
> * gcc.target/i386/sse-22.c: Ditto..
> * gcc.target/i386/sse-33.c: Ditto..
> * gcc.target/i386/wrpku-1.c: New test.
>
> +(define_expand "rdpkru"
> + [(set (match_operand:SI 0 "register_operand")
> + (unspec:SI [(const_int 0)] UNSPEC_PKU))
> + (set (reg:SI CX_REG)
> + (const_int 0))
> + (clobber (reg:SI DX_REG))]
> + "TARGET_PKU"
> +{
> + emit_move_insn (gen_rtx_REG (SImode, CX_REG), CONST0_RTX (SImode));
> + emit_insn (gen_rdpkru_2 (operands[0]));
> + DONE;
> +})
You should use "parallel" to emit insn with several parallel
expressions. So, in the preparation statements, you move const0 to a
pseudo, so the RA will later use correct register. And please leave to
the expander to emit the pattern.
> +(define_insn "rdpkru_2"
> + [(set (match_operand:SI 0 "register_operand" "=a")
> + (unspec:SI [(const_int 0)] UNSPEC_PKU))
> + (clobber (reg:SI DX_REG))
> + (use (reg:SI CX_REG))]
> + "TARGET_PKU"
> + "rdpkru"
> + [(set_attr "type" "other")])
Please do not use explicit hard registers. There are appropriate
single-reg constraints available for use. Without seeing the
documentation, I think the above should look like:
(define_insn "*rdpkru"
[(set (match_operand:SI 0 "register_operand" "=a")
(unspec:SI [(match_operand:SI 1 "register_operand" "c")] UNSPEC_PKU))
(clobber (rmatch_operand "register_operand "=d"))
"TARGET_PKU"
"rdpkru"
[(set_attr "type" "other")])
> +(define_expand "wrpkru"
> + [(unspec_volatile:SI [(match_operand:SI 0 "register_operand")] UNSPECV_PKU)
> + (set (reg:SI CX_REG)
> + (const_int 0))
> + (set (reg:SI DX_REG)
> + (const_int 0))]
> + "TARGET_PKU"
> +{
> + emit_move_insn (gen_rtx_REG (SImode, CX_REG), CONST0_RTX (SImode));
> + emit_move_insn (gen_rtx_REG (SImode, DX_REG), CONST0_RTX (SImode));
> + emit_insn (gen_wrpkru_2 (operands[0]));
> + DONE;
> +})
> +
> +(define_insn "wrpkru_2"
> + [(unspec_volatile:SI [(match_operand:SI 0 "register_operand" "a")]
> UNSPECV_PKU)
> + (use (reg:SI CX_REG))
> + (use (reg:SI DX_REG))]
> + "TARGET_PKU"
> + "wrpkru"
> + [(set_attr "type" "other")])
>
Please move all input operands to the insisde of the unspec, but it
looks that this pattern is missing clobber, as in the above rdpkru
pattern.
Uros.
