Hi!
On Wed, Sep 03, 2025 at 07:50:32PM +0530, Avinash Jayakar wrote:
> On Fri, 2025-08-29 at 13:14 -0500, Segher Boessenkool wrote:
> > 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.
At least combine will merge that together with the move from the
const_int. What happened there? The dump file will tell you.
> If possible, could you please suggest how I can recognize
> operand #2 as a constant when it is already in a register.
Just as the const_int thing.
> 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;
> + }
> +})
CONSTANT_P is much more general than what you want here. Just check for
const_int more directly?
But, don't do that at all! Tou do not want to use INTVAL, you want to
check if the node is equal to const1_rtx directly! That is the
canonical representation for (const_int 1), it is a unique RTX, wherever
(const_int 1) is used it always will use const1_rtx. As it says in
emit-rtl.cc:
> /* We make one copy of (const_int C) where C is in
> [- MAX_SAVED_CONST_INT, MAX_SAVED_CONST_INT]
> to save space during the compilation and simplify comparisons of
> integers. */
>
> rtx const_int_rtx[MAX_SAVED_CONST_INT * 2 + 1];
(and rtl.h has
> #define const1_rtx (const_int_rtx[MAX_SAVED_CONST_INT+1])
which ties it all neatly together). So comparing something against
const1_rtx makes sure it is a const_int, and that its value is 1, all in
one fell swoop. RTL can be very convenient to use :-)
> As you mentioned I will have to add patterns for various sizes as well.
Some judicious use of mode_iterator's should help :-) It probably is
easiest to write one first, the vaddudm perhaps, but the more general
thing is easier to test, etc. :-)
Segher
