Hi!
On Thu, Aug 14, 2025 at 11:27:10AM +0530, Avinash Jayakar wrote:
> Below is a draft of the patch for PR119702. I request you to
> please review it.
I assume it is just a proposed patch for trunk, and it will allow you to
resolve that PR? :-)
> In vector extensions for rs6000, there is no immediate version
> of left shift.
Yeah. That would cost a lot of opcode space. Perhaps we should have
one nowadays, as a prefixed insn?
> This leads to having 2 instructions for the simple
> case of left shift by one.
> vspltisw 0,1
> vsld 2,2,0
> This could have been performed simply with one add instruction
> vaddudm 2,2,2
> This patch fixes this issue. During the expansion of vashl op
> check if the operand number 2 is a constant and its value is 1,
> if yes then generate plus op, otherwise materialize the result
> of operand 2 into a register and generate ashift op.
Please don't do this. Just do a define_insn that recognises a shift by
const int 1, and emits an vaddudm machine insn for that. The RTL as we
currently create is just fine, and if we would want a canonical
representation for this the shift is probably the best idea!
> PR target/119702
> gcc:
> * config/rs6000/vector.md (vashl<mode>3): Generate add when
> operand 2 is a constant with value 1.
Changelog lines are 80 character positions wide. Please do not wrap
early.
There should be no indent for continuation lines after the * line. So:
* wherever/that/is: Something and something more more more more more
more more more and some more.
> diff --git a/gcc/config/rs6000/vector.md b/gcc/config/rs6000/vector.md
> index f5797387ca7..02e2361e4a3 100644
> --- a/gcc/config/rs6000/vector.md
> +++ b/gcc/config/rs6000/vector.md
> @@ -1391,9 +1391,29 @@
> (define_expand "vashl<mode>3"
> [(set (match_operand:VEC_I 0 "vint_operand")
> (ashift:VEC_I (match_operand:VEC_I 1 "vint_operand")
> - (match_operand:VEC_I 2 "vint_operand")))]
> + (match_operand:VEC_I 2 "general_operand")))]
> "VECTOR_UNIT_ALTIVEC_OR_VSX_P (<MODE>mode)"
> - "")
> +{
> + rtx op2 = operands[2];
> + if (CONSTANT_P(op2)) {
> + HOST_WIDE_INT shift = INTVAL(const_vector_elt (op2, 0));
> +
> + if (shift == 1)
> + {
> + emit_insn (gen_rtx_SET (operands[0],
> + gen_rtx_PLUS (<MODE>mode,
> + operands[1],
> + operands[1])));
> + DONE;
> + }
> + }
> + operands[2] = copy_to_mode_reg (<MODE>mode, op2);
> + emit_insn (gen_rtx_SET (operands[0],
> + gen_rtx_ASHIFT (<MODE>mode,
> + operands[1],
> + operands[2])));
> + DONE;
> +})
You shouldn't ever do things this way: if what you want to generate is
just the pattern of the define_*, just fall throough (without calling
DONE (or FAIL). This is much more usual in a define_insn, but it works
fine in a define_expand as well, see our add<mode>3_carry_in for example
(this pattern is generated by name from other things in the backend,
many of those things will finally end up in define_insn
*add<mode>3_carry_in_internal, the next pattern, doing "adde").
But, as I started this email, just add a define_insn that recognises a
shift by const_int 1 (for many sizes btw, not just 64!), and emits an
add to self for it. There is no need to change the RTL for it, the
existing RTL is a perfectly fine description of it, and although there
is no canonical representation for it (except mul by two is explicitly
not canonical) it clearly is the simplest way to write things.
> 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..2f53a26eaaf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr119702-1.c
> @@ -0,0 +1,40 @@
> +/* { dg-do compile } */
> +/* { dg-options "-mdejagnu-cpu=power10 -O2" } */
Why this? vaddudm is an arch 2.07 insn, p8, 2013.
> +/* { dg-require-effective-target lp64 } */
Why? What does 64-bit *addressing* have to do with this at all?
Maybe you want the powerpc64 selector? Which is implied by your -mcpu=
of course.
> +/* { dg-require-effective-target powerpc_vsx } */
And why this? Esp. if you already have an -mcpu= that guarantees VSX
insn generation will be enabled :-)
> +#include <altivec.h>
> +#define ull unsigned long long
> +
> +void lshift1(unsigned long long *a) {
Why define "ull" and then not use it in most places? Missed chance :-)
> + a[0] <<= 1;
> + a[1] <<= 1;
> +}
> +
> +vector ull lshift1_vector(vector ull a) {
> + return a <<= 1;
> +}
> +
> +void add(unsigned long long *a)
> +{
> + a[0] += a[0];
> + a[1] += a[1];
> +}
> +
> +vector ull add_vector(vector ull a) {
> + return a + a;
> +}
> +
> +void mult2(unsigned long long *a)
> +{
> + a[0] *= 2;
> + a[1] *= 2;
> +}
> +
> +vector ull mult2_vector(vector ull a)
> +{
> + return a*2;
> +}
> +
> +/* { dg-final { scan-assembler-times {\mvaddudm?\M} 6 } } */
Why the "?" in "m?". There does not exist any insn without a "m" here!
Maybe you can make the testcase work for b, h, w as well?
Segher
