Thank you for reviewing this patch!
On Fri, 2025-08-29 at 13:14 -0500, Segher Boessenkool wrote:
> > 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!
>
I did try this approach, and encountered several issues.
1. The pattern
[(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")))]
makes sure that operand #2 must be vint_operand, thus if it is a
constant, then the function 'maybe_legitimize_operands' which is called
under 'maybe_gen_insn', materialized the constant into a register after
which the pattern matches successfully.
2. Due to the above problem, I could not create a define insn, that can
look at the operand 2 as a constant and generate vaddudm. I try
something like following:
(define_insn
[(set (match_operand:VEC_I 0 "vint_operand")
(ashift:VEC_I (match_operand:VEC_I 1 "vint_operand")
(const_vector:V2DI [(const_int 1) (const_int
1)])))]
"<VI_unit>"
{
return "vaddu<VI_char>m %0,%1,%1";
}
)
But the problem is again, operand 2 is always materialized into a
register. If possible, could you please suggest how I can recognize
operand #2 as a constant when it is already in a register.
> > 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").
>
Just adding this comment, in case the above mentioned approach does not
work. Would this be ok? Make the operand #2 constraint as
general_operand (so that it is not materialized into a register). Then
if operand #2 is a constant int 1, pass through, else copy it to mode
reg and generate emit ASHIFT insn.
diff --git a/gcc/config/rs6000/vector.md b/gcc/config/rs6000/vector.md
index f5797387ca7..cba57a7319e 100644
--- a/gcc/config/rs6000/vector.md
+++ b/gcc/config/rs6000/vector.md
@@ -1387,13 +1387,24 @@
DONE;
})
+
;; Expanders for arithmetic shift left on each vector element
(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) && INTVAL(const_vector_elt (op2, 0)) != 1) {
+ 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;
+ }
+})
This way it is easy to recognize in define_insn as follows
diff --git a/gcc/config/rs6000/altivec.md
b/gcc/config/rs6000/altivec.md
index 7edc288a656..3f678f2e666 100644
--- a/gcc/config/rs6000/altivec.md
+++ b/gcc/config/rs6000/altivec.md
@@ -2107,6 +2107,17 @@
"vsrv %0,%1,%2"
[(set_attr "type" "vecsimple")])
+
+(define_insn ""
+ [(set (match_operand:VI2 0 "register_operand" "=v")
+ (ashift:VI2 (match_operand:VI2 1 "register_operand" "v")
+ (const_vector:V2DI [(const_int 1) (const_int
1)])))]
+ "<VI_unit>"
+ {
+ return "vaddu<VI_char>m %0,%1,%1";
+ }
+)
+
As you mentioned I will have to add patterns for various sizes as well.
Regards,
Avinash Jayakar
