Hi Avinash,
On 04/09/25 3:32 pm, Avinash Jayakar wrote:
> Hello,
>
> This is the second version of the patch proposed for master aiming to fix
> PR119702. I request the review of this patch.
Whenever we send a new version, we should mention what has changed in
this version. Something like:
Changes in v2: <describe the changes>
>
> The following sequence of assembly in powerpc64le
> vspltisw 0,1
> vsld 2,2,0
> is replaced by this
> vaddudm 2,2,2
> whenever there is a vector left shift by a constant value 1.
> Added the pattern in altivec.md to recognize a vector left shift by a
> constant
> value, and generate add instructions if constant is 1.
>
> PR target/119702
> gcc:
> * config/rs6000/altivec.md: Added a define_insn to recognize left shift
The line above should start after a tab which is 8 spaces wide.
Ditto for the rest of the changelog.
> by constant 1 for vector instructions.
> * config/rs6000/predicates.md(shift_constant_1): Added a predicate for
> detecting if all values in a const_vector are 1.
> gcc/testsuite:
> * gcc.target/powerpc/pr119702-1.c: New test for (check generation of vadd
> for different integer types)
> ---
> gcc/config/rs6000/altivec.md | 8 +++
> gcc/config/rs6000/predicates.md | 11 ++++
> gcc/testsuite/gcc.target/powerpc/pr119702-1.c | 54 +++++++++++++++++++
> 3 files changed, 73 insertions(+)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr119702-1.c
>
> diff --git a/gcc/config/rs6000/altivec.md b/gcc/config/rs6000/altivec.md
> index 7edc288a656..8b1beb8f713 100644
> --- a/gcc/config/rs6000/altivec.md
> +++ b/gcc/config/rs6000/altivec.md
> @@ -2107,6 +2107,14 @@
> "vsrv %0,%1,%2"
> [(set_attr "type" "vecsimple")])
>
> +(define_insn ""
Provide a name prefixed with the '*' character for the define_insn.
> + [(set (match_operand:VI2 0 "register_operand" "=v")
> + (ashift:VI2 (match_operand:VI2 1 "register_operand" "v")
> + (match_operand:VI2 2 "shift_constant_1" "")))]
> + "<VI_unit>"
> + "vaddu<VI_char>m %0,%1,%1"
> +)
> +
> (define_insn "*altivec_vsl<VI_char>"
> [(set (match_operand:VI2 0 "register_operand" "=v")
> (ashift:VI2 (match_operand:VI2 1 "register_operand" "v")
> diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
> index 647e89afb6a..e791ea2d009 100644
> --- a/gcc/config/rs6000/predicates.md
> +++ b/gcc/config/rs6000/predicates.md
> @@ -924,6 +924,17 @@
> }
> })
>
> +(define_predicate "shift_constant_1"
> + (match_code "const_vector")
> +{
> + unsigned nunits = GET_MODE_NUNITS (mode), i;
> + for (i = 1; i < nunits; i++) {
The loop is not iterating over all elements of the vector. The 0th
element is missed.
The brace '{' is not needed. Also, the brace is not following the
coding convention in the rest of the file, namely the brace appears on
the next line after 'for'.
> + if (INTVAL (CONST_VECTOR_ELT(op, i)) != 1)
> + return 0;
> + }
> + return 1;
> +})
> +
> ;; Return 1 if operand is 0.0.
> (define_predicate "zero_fp_constant"
> (and (match_code "const_double")
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr119702-1.c
> b/gcc/testsuite/gcc.target/powerpc/pr119702-1.c
> new file mode 100644
> index 00000000000..6c011fdb72f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr119702-1.c
> @@ -0,0 +1,54 @@
> +/* { dg-do compile } */
> +/* { dg-options "-mdejagnu-cpu=power8 -O2" } */
> +
> +#include <inttypes.h>
> +#define ull unsigned long long
Why define a type that is not being used?
-Surya
