Hi Carl,
on 2023/5/3 23:30, Carl Love via Gcc-patches wrote:
> GCC maintainers:
>
> The following patch cleans up the definition for the
> __builtin_altivec_vcmpnet. The current implementation implies that the
> builtin is only supported on Power 9 since it is defined under the
> Power 9 stanza. However the builtin has no ISA restrictions as stated
> in the Power Vector Intrinsic Programming Reference document. The
> current built-in works because the builtin gets replaced during GIMPLE
> folding by a simple not-equal operator so it doesn't get expanded and
> checked for Power 9 code generation.
>
> This patch moves the definition to the Altivec stanza in the builtin
> definition file. The builtin then generates code for Power 8 and
> earlier processors or Power 9 and later processors.
>
> The patch has been tested on Power 8 and Power 9 with no regressions.
>
> Please let me know if the patch is acceptable for mainline. Thanks.
>
> Carl
>
> -----------------------------------------
> rs6000: vec_cmpne confusing implementation
A better subject seems to be "Make __builtin_altivec_vcmpne{b,h,w,t}
supported under altivec" to match better what this patch does?
>
> __builtin_altivec_vcmpnet does not have any ISA restrictions. The current
Power Vector Intrinsic Programming Reference doesn't define functions
__builtin_altivec_vcmpne* but only vec_cmpne, which hasn't restricted any ISA
yet.
So I think what you proposed is actually to extend the current built-in
functions
__builtin_altivec_vcmpne{b,h,w,t} not longer power9 and later only.
This patch needs some test cases for the corresponding changes, but I'm curious
why people want to use these built-ins instead of just using vec_cmpne? The
latter looks more general and better.
> built-in definitions for vcmpneb, vcmpneh, vcmpnew, vcmpnet are defined
> under the Power 9 section. This implies they are only supported on Power
> 9 and above when in fact they are defined and work on Power 8.
>
> This patch moves the definitions to the Altivec stanza and maps the
> builtin dispatches to different code generation for Power 8 and earlier
> or for Power 9 and later.
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000-builtins.def (vcmpneb, vcmpneh, vcmpnew.
> vcmpnet): Move definitions to Altivec stanza.
> * config/rs6000/vsx.md (cmpneb, vcmpneh, vcmpnew, vcmpnet): New
> define_expand.
> (cmpneb, vcmpneh, vcmpnew, vcmpnet): Rename define_insn.
>
> Patch has been tested on Power 8 and Power 9 with no regressions.
> ---
> gcc/config/rs6000/rs6000-builtins.def | 24 ++++++------
> gcc/config/rs6000/vsx.md | 54 +++++++++++++++++++++++++--
> 2 files changed, 63 insertions(+), 15 deletions(-)
>
> diff --git a/gcc/config/rs6000/rs6000-builtins.def
> b/gcc/config/rs6000/rs6000-builtins.def
> index 638d0bc72ca..adb4122be29 100644
> --- a/gcc/config/rs6000/rs6000-builtins.def
> +++ b/gcc/config/rs6000/rs6000-builtins.def
> @@ -641,6 +641,18 @@
> const int __builtin_altivec_vcmpgtuw_p (int, vsi, vsi);
> VCMPGTUW_P vector_gtu_v4si_p {pred}
>
> + const vsc __builtin_altivec_vcmpneb (vsc, vsc);
> + VCMPNEB vcmpneb {}
> +
> + const vss __builtin_altivec_vcmpneh (vss, vss);
> + VCMPNEH vcmpneh {}
> +
> + const vsi __builtin_altivec_vcmpnew (vsi, vsi);
> + VCMPNEW vcmpnew {}
> +
The expanders for these three can just start with
separated eq and not, later if vcmpne{b,h,w} get
supported, these two can be combined further.
> + const vbq __builtin_altivec_vcmpnet (vsq, vsq);
> + VCMPNET vcmpnet {}
The current define_expand for vcmpnet guards TARGET_POWER10,
once you have test cases to cover these changes, you will found
the current expander isn't enough for what you want.
> +
> const vsi __builtin_altivec_vctsxs (vf, const int<5>);
> VCTSXS altivec_vctsxs {}
>
> @@ -2599,9 +2611,6 @@
> const signed int __builtin_altivec_vcmpaew_p (vsi, vsi);
> VCMPAEW_P vector_ae_v4si_p {}
>
> - const vsc __builtin_altivec_vcmpneb (vsc, vsc);
> - VCMPNEB vcmpneb {}
> -
> const signed int __builtin_altivec_vcmpneb_p (vsc, vsc);
> VCMPNEB_P vector_ne_v16qi_p {}
>
> @@ -2614,15 +2623,9 @@
> const signed int __builtin_altivec_vcmpnefp_p (vf, vf);
> VCMPNEFP_P vector_ne_v4sf_p {}
>
> - const vss __builtin_altivec_vcmpneh (vss, vss);
> - VCMPNEH vcmpneh {}
> -
> const signed int __builtin_altivec_vcmpneh_p (vss, vss);
> VCMPNEH_P vector_ne_v8hi_p {}
>
> - const vsi __builtin_altivec_vcmpnew (vsi, vsi);
> - VCMPNEW vcmpnew {}
> -
> const signed int __builtin_altivec_vcmpnew_p (vsi, vsi);
> VCMPNEW_P vector_ne_v4si_p {}
>
> @@ -3203,9 +3206,6 @@
> const signed int __builtin_altivec_vcmpgtut_p (signed int, vuq, vuq);
> VCMPGTUT_P vector_gtu_v1ti_p {pred}
>
> - const vbq __builtin_altivec_vcmpnet (vsq, vsq);
> - VCMPNET vcmpnet {}
> -
> const signed int __builtin_altivec_vcmpnet_p (vsq, vsq);
> VCMPNET_P vector_ne_v1ti_p {}
>
> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
> index 7d845df5c2d..3f05e3e6d00 100644
> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -5652,8 +5652,56 @@
> DONE;
> })
>
> +;; Expand for builtin vcmpneb
> +(define_expand "vcmpneb"
> + [(set (match_operand:V16QI 0 "altivec_register_operand" "=v")
> + (not:V16QI
> + (eq:V16QI (match_operand:V16QI 1 "altivec_register_operand" "v")
> + (match_operand:V16QI 2 "altivec_register_operand" "v"))))]
> + ""
> + {
> + if (TARGET_P9_VECTOR)
> + emit_insn (gen_vcmpneb_p9 (operands[0], operands[1], operands[2]));
> + else
> + emit_insn (gen_altivec_vcmpequb_p (operands[0], operands[1],
> + operands[2]));
> + DONE;
> + })
> +
It's better to have an explicit condition TARGET_ALTIVEC to match the stanza.
As mentioned above, you can just expand it with eq and not with something like:
(define_expand "vcmpneb"
[(set (match_operand:V16QI 3 "altivec_register_operand" "=v")
(eq:V16QI (match_operand:V16QI 1 "altivec_register_operand" "v")
(match_operand:V16QI 2 "altivec_register_operand" "v")))
(set (match_operand:V16QI 0 "altivec_register_operand" "=v")
(not:V16QI (match_dup 3))]
"TARGET_ALTIVEC"
{
operands[3] = gen_reg_rtx (V16QImode);
});
And move these expands to altivec.md since this is not vsx specific.
> +;; Expand for builtin vcmpneh
> +(define_expand "vcmpneh"
> + [(set (match_operand:V8HI 0 "altivec_register_operand" "=v")
> + (not:V8HI
> + (eq:V8HI (match_operand:V8HI 1 "altivec_register_operand" "v")
> + (match_operand:V8HI 2 "altivec_register_operand" "v"))))]
> + ""
> + {
> + if (TARGET_P9_VECTOR)
> + emit_insn (gen_vcmpneh_p9 (operands[0], operands[1], operands[2]));
> + else
> + emit_insn (gen_altivec_vcmpequh_p (operands[0], operands[1],
> + operands[2]));
> + DONE;
> + })
> +
Ditto,
> +;; Expand for builtin vcmpnew
> +(define_expand "vcmpnew"
> + [(set (match_operand:V4SI 0 "altivec_register_operand" "=v")
> + (not:V4SI
> + (eq:V4SI (match_operand:V4SI 1 "altivec_register_operand" "v")
> + (match_operand:V4SI 2 "altivec_register_operand" "v"))))]
> + ""
> + {
> + if (TARGET_P9_VECTOR)
> + emit_insn (gen_vcmpnew_p9 (operands[0], operands[1], operands[2]));
> + else
> + emit_insn (gen_altivec_vcmpequw_p (operands[0], operands[1],
> + operands[2]));
> + DONE;
> + })
Ditto, they can be merged with some mode iterators further.
> +
> ;; Vector Compare Not Equal Byte (specified/not+eq:)
> -(define_insn "vcmpneb"
> +(define_insn "vcmpneb_p9"
> [(set (match_operand:V16QI 0 "altivec_register_operand" "=v")
> (not:V16QI
> (eq:V16QI (match_operand:V16QI 1 "altivec_register_operand" "v")
> @@ -5703,7 +5751,7 @@
> [(set_attr "type" "vecsimple")])
>
> ;; Vector Compare Not Equal Half Word (specified/not+eq:)
> -(define_insn "vcmpneh"
> +(define_insn "vcmpneh_p9"
> [(set (match_operand:V8HI 0 "altivec_register_operand" "=v")
> (not:V8HI
> (eq:V8HI (match_operand:V8HI 1 "altivec_register_operand" "v")
> @@ -5723,7 +5771,7 @@
> [(set_attr "type" "vecsimple")])
>
> ;; Vector Compare Not Equal Word (specified/not+eq:)
> -(define_insn "vcmpnew"
> +(define_insn "vcmpnew_p9"
> [(set (match_operand:V4SI 0 "altivec_register_operand" "=v")
> (not:V4SI
> (eq:V4SI (match_operand:V4SI 1 "altivec_register_operand" "v")
These are not needed with the above changes.
BR,
Kewen
