[v2] RISC-V: Fix ICE for unrecognizable insn `UNSPEC_VSETVL` for XTheadVector

  Since XTheadvector does not support vsetivli, vl needs to be put into
registers during the expand phase.

	PR 116593

gcc/ChangeLog:

	* config/riscv/riscv-vector-builtins.cc (function_expander::add_input_operand):
	Put const to GPR for vl.
	* config/riscv/thead-vector.md (@th_pred_vl_mov<mode>): New.

gcc/testsuite/ChangeLog:

	* g++.target/riscv/xtheadvector/pr116593.C: New test.
	* g++.target/riscv/xtheadvector/xtheadvector.exp: New test.

Reported-by: nihui <shuizhuyuanluo@gmail.com>
---
 gcc/config/riscv/riscv-vector-builtins.cc     | 18 +++++++-
 gcc/config/riscv/thead-vector.md              | 13 ++++++
 .../g++.target/riscv/xtheadvector/pr116593.C  | 45 +++++++++++++++++++
 .../riscv/xtheadvector/xtheadvector.exp       | 37 +++++++++++++++
 4 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.target/riscv/xtheadvector/pr116593.C
 create mode 100644 gcc/testsuite/g++.target/riscv/xtheadvector/xtheadvector.exp
  

On 12/2/24 11:34 PM, Jin Ma wrote:
> Since XTheadvector does not support vsetivli, vl needs to be put into
> registers during the expand phase.
> 
> 	PR 116593
> 
> gcc/ChangeLog:
> 
> 	* config/riscv/riscv-vector-builtins.cc (function_expander::add_input_operand):
> 	Put const to GPR for vl.
> 	* config/riscv/thead-vector.md (@th_pred_vl_mov<mode>): New.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.target/riscv/xtheadvector/pr116593.C: New test.
> 	* g++.target/riscv/xtheadvector/xtheadvector.exp: New test.
> 
> Reported-by: nihui <shuizhuyuanluo@gmail.com>
> ---
>   gcc/config/riscv/riscv-vector-builtins.cc     | 18 +++++++-
>   gcc/config/riscv/thead-vector.md              | 13 ++++++
>   .../g++.target/riscv/xtheadvector/pr116593.C  | 45 +++++++++++++++++++
>   .../riscv/xtheadvector/xtheadvector.exp       | 37 +++++++++++++++
>   4 files changed, 112 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.target/riscv/xtheadvector/pr116593.C
>   create mode 100644 gcc/testsuite/g++.target/riscv/xtheadvector/xtheadvector.exp
> 
> diff --git a/gcc/config/riscv/riscv-vector-builtins.cc b/gcc/config/riscv/riscv-vector-builtins.cc
> index b9b9d33adab6..cced0461a7bb 100644
> --- a/gcc/config/riscv/riscv-vector-builtins.cc
> +++ b/gcc/config/riscv/riscv-vector-builtins.cc
> @@ -4089,7 +4089,23 @@ function_expander::add_input_operand (unsigned argno)
>   {
>     tree arg = CALL_EXPR_ARG (exp, argno);
>     rtx x = expand_normal (arg);
> -  add_input_operand (TYPE_MODE (TREE_TYPE (arg)), x);
> +
> +  /* Since the parameter vl of XTheadVector does not support
> +     immediate numbers, we need to put it in the register
> +     in advance.  */
> +  if (TARGET_XTHEADVECTOR
> +      && CONST_INT_P (x)
> +      && base->apply_vl_p ()
> +      && argno == (unsigned) (call_expr_nargs (exp) - 1)
> +      && x != CONST0_RTX (GET_MODE (x)))
> +    {
> +      rtx tmp = gen_reg_rtx (word_mode);
> +      /* Use UNSPEC to avoid being optimized before vsetvl pass.  */
> +      emit_insn (gen_th_pred_vl_mov (word_mode, tmp, x));
> +      add_input_operand (TYPE_MODE (TREE_TYPE (arg)), tmp);
> +    }
> +  else
> +    add_input_operand (TYPE_MODE (TREE_TYPE (arg)), x);
>   }
So I would just do:


tmp = force_reg (word_mode, x);
add_input_operand (TYPE_MODE (TREE_TYPE (arg)), tmp);

In the thead specific code.  That generates the initial code correctly. 
At that point we just need to make sure nothing like combine, cprop, etc 
propagates the constant into the vsetvl.  The way to prevent that is 
with the operand predicates on the vsetvl insns.


jeff
  

> So I would just do:
> 
> 
> tmp = force_reg (word_mode, x);
> add_input_operand (TYPE_MODE (TREE_TYPE (arg)), tmp);
> 
> In the thead specific code. That generates the initial code correctly.
> At that point we just need to make sure nothing like combine, cprop, etc
> propagates the constant into the vsetvl. The way to prevent that is
> with the operand predicates on the vsetvl insns.
>
> jeff
Thank you so much for your patient guidance. I understand what you mean and
will update the patch accordingly :)
BR
Jin
  

> > --- a/gcc/config/riscv/riscv-vector-builtins.cc
> > +++ b/gcc/config/riscv/riscv-vector-builtins.cc
> > @@ -4089,7 +4089,23 @@ function_expander::add_input_operand (unsigned argno)
> >   {
> >     tree arg = CALL_EXPR_ARG (exp, argno);
> >     rtx x = expand_normal (arg);
> > -  add_input_operand (TYPE_MODE (TREE_TYPE (arg)), x);
> > +
> > +  /* Since the parameter vl of XTheadVector does not support
> > +     immediate numbers, we need to put it in the register
> > +     in advance.  */
> > +  if (TARGET_XTHEADVECTOR
> > +      && CONST_INT_P (x)
> > +      && base->apply_vl_p ()
> > +      && argno == (unsigned) (call_expr_nargs (exp) - 1)
> > +      && x != CONST0_RTX (GET_MODE (x)))
> > +    {
> > +      rtx tmp = gen_reg_rtx (word_mode);
> > +      /* Use UNSPEC to avoid being optimized before vsetvl pass.  */
> > +      emit_insn (gen_th_pred_vl_mov (word_mode, tmp, x));
> > +      add_input_operand (TYPE_MODE (TREE_TYPE (arg)), tmp);
> > +    }
> > +  else
> > +    add_input_operand (TYPE_MODE (TREE_TYPE (arg)), x);
> >   }
> So I would just do:
> 
> 
> tmp = force_reg (word_mode, x);
> add_input_operand (TYPE_MODE (TREE_TYPE (arg)), tmp);
> 
> In the thead specific code.  That generates the initial code correctly. 
> At that point we just need to make sure nothing like combine, cprop, etc 
> propagates the constant into the vsetvl.  The way to prevent that is 
> with the operand predicates on the vsetvl insns.
> 
> 
> jeff

Hi, jeff

I missed some information because of the long time.

As I said before, instead of using UNSPEC, In the curr_insn_transform function, the insn is transformed from:
(insn 69 67 225 12 (set (mem:RVVM8SF (reg/f:DI 218 [ _77 ]) [0  S[128, 128] A32])
        (if_then_else:RVVM8SF (unspec:RVVMF4BI [
                    (const_vector:RVVMF4BI repeat [
                            (const_int 1 [0x1])
                        ])
                    (reg:DI 209)
                    (const_int 0 [0])
                    (reg:SI 66 vl)
                    (reg:SI 67 vtype)
                ] UNSPEC_VPREDICATE)
            (reg/v:RVVM8SF 143 [ _xx ])
            (mem:RVVM8SF (reg/f:DI 218 [ _77 ]) [0  S[128, 128] A32]))) "pr116593.C":14:24 discrim 1 3883 {pred_storervvm8sf}
     (expr_list:REG_DEAD (reg/v:RVVM8SF 143 [ _xx ])
        (nil)))
to
(insn 69 284 225 11 (set (mem:RVVM8SF (reg/f:DI 18 s2 [orig:218 _77 ] [218]) [0  S[128, 128] A32])
        (if_then_else:RVVM8SF (unspec:RVVMF4BI [
                    (const_vector:RVVMF4BI repeat [
                            (const_int 1 [0x1])
                        ])
                    (const_int 1 [0x1])
                    (const_int 0 [0])
                    (reg:SI 66 vl)
                    (reg:SI 67 vtype)
                ] UNSPEC_VPREDICATE)
            (reg/v:RVVM8SF 104 v8 [orig:143 _xx ] [143])
            (mem:RVVM8SF (reg/f:DI 18 s2 [orig:218 _77 ] [218]) [0  S[128, 128] A32]))) "pr116593.C":14:24 discrim 1 3883 {pred_storervvm8sf}
     (nil))

Looking at the log for the reload pass, it is found that "Changing pseudo 209 in operand 3 of insn 69 on equiv 0x 1".
It converts the vl operand in insn from the expected register(reg:DI 209) to the constant 1(const_int 1 [0x1]).

This conversion occurs because, although the predicate for the vl operand is restricted by "vector_length_operand" in the pattern, 
the constraint is still "rK", which allows the transformation.

The issue is that changing the "rK" constraint to "rJ" for the constraint of vl operand in the pattern would prevent this conversion,
But unfortunately this will conflict with RVV (RISC-V Vector Extension).

This is why I initially considered using UNSPEC to address the XTheadVector problem while minimizing interference with RVV.

I'm not sure if there is a better way, do you have any suggestions?

BR
Jin
  

> Looking at the log for the reload pass, it is found that "Changing pseudo 209
> in operand 3 of insn 69 on equiv 0x 1". It converts the vl operand in insn
> from the expected register(reg:DI 209) to the constant 1(const_int 1 [0x1]).
>
> This conversion occurs because, although the predicate for the vl operand is
> restricted by "vector_length_operand" in the pattern, the constraint is still
> "rK", which allows the transformation.
>
> The issue is that changing the "rK" constraint to "rJ" for the constraint of
> vl operand in the pattern would prevent this conversion, But unfortunately
> this will conflict with RVV (RISC-V Vector Extension).
>
> This is why I initially considered using UNSPEC to address the XTheadVector
> problem while minimizing interference with RVV.
>
> I'm not sure if there is a better way, do you have any suggestions?

We'd probably need to disable the alternatives via the "spec_restriction"
attribute as we do for the fma ops.
  

On 12/17/24 5:05 AM, Jin Ma wrote:

> 
> I missed some information because of the long time.
Happens to me all the time ;-)


> 
> As I said before, instead of using UNSPEC, In the curr_insn_transform function, the insn is transformed from:
> (insn 69 67 225 12 (set (mem:RVVM8SF (reg/f:DI 218 [ _77 ]) [0  S[128, 128] A32])
>          (if_then_else:RVVM8SF (unspec:RVVMF4BI [
>                      (const_vector:RVVMF4BI repeat [
>                              (const_int 1 [0x1])
>                          ])
>                      (reg:DI 209)
>                      (const_int 0 [0])
>                      (reg:SI 66 vl)
>                      (reg:SI 67 vtype)
>                  ] UNSPEC_VPREDICATE)
>              (reg/v:RVVM8SF 143 [ _xx ])
>              (mem:RVVM8SF (reg/f:DI 218 [ _77 ]) [0  S[128, 128] A32]))) "pr116593.C":14:24 discrim 1 3883 {pred_storervvm8sf}
>       (expr_list:REG_DEAD (reg/v:RVVM8SF 143 [ _xx ])
>          (nil)))
> to
> (insn 69 284 225 11 (set (mem:RVVM8SF (reg/f:DI 18 s2 [orig:218 _77 ] [218]) [0  S[128, 128] A32])
>          (if_then_else:RVVM8SF (unspec:RVVMF4BI [
>                      (const_vector:RVVMF4BI repeat [
>                              (const_int 1 [0x1])
>                          ])
>                      (const_int 1 [0x1])
>                      (const_int 0 [0])
>                      (reg:SI 66 vl)
>                      (reg:SI 67 vtype)
>                  ] UNSPEC_VPREDICATE)
>              (reg/v:RVVM8SF 104 v8 [orig:143 _xx ] [143])
>              (mem:RVVM8SF (reg/f:DI 18 s2 [orig:218 _77 ] [218]) [0  S[128, 128] A32]))) "pr116593.C":14:24 discrim 1 3883 {pred_storervvm8sf}
>       (nil))
> 
> Looking at the log for the reload pass, it is found that "Changing pseudo 209 in operand 3 of insn 69 on equiv 0x 1".
> It converts the vl operand in insn from the expected register(reg:DI 209) to the constant 1(const_int 1 [0x1]).
OK.  This is where we need to focus.

> 
> This conversion occurs because, although the predicate for the vl operand is restricted by "vector_length_operand" in the pattern,
> the constraint is still "rK", which allows the transformation.
That's a bit odd, I would have expected the predicate to be checked, but 
sometimes things are "odd" in reload.

> 
> The issue is that changing the "rK" constraint to "rJ" for the constraint of vl operand in the pattern would prevent this conversion,
> But unfortunately this will conflict with RVV (RISC-V Vector Extension).
> 
> This is why I initially considered using UNSPEC to address the XTheadVector problem while minimizing interference with RVV.
> 
> I'm not sure if there is a better way, do you have any suggestions?
I think Robin mentioned disabling certain alternatives which is one 
approach (I've never used that capability, but I know it exists and this 
would be a pretty good fit).  For this approach I'd suggest disabling 
the rK alternative for thead-vector and (if we don't have it) adding an 
alternative that only accepts a register.

We could also create a new constraint that mostly behaves like rK, but 
rejects (const_int 1) when thead-vector is enabled and use that in the 
vsetvl pattern instead of rK.

The brute force, but least favorite, approach would be to create two 
distinct patterns, one for thead the other for rvv.  Obviously the thead 
pattern would use different constraints to prevent replacing registers 
with constants.

Jeff

Jeff
  

> > Looking at the log for the reload pass, it is found that "Changing pseudo 209
> > in operand 3 of insn 69 on equiv 0x 1". It converts the vl operand in insn
> > from the expected register(reg:DI 209) to the constant 1(const_int 1 [0x1]).
> >
> > This conversion occurs because, although the predicate for the vl operand is
> > restricted by "vector_length_operand" in the pattern, the constraint is still
> > "rK", which allows the transformation.
> >
> > The issue is that changing the "rK" constraint to "rJ" for the constraint of
> > vl operand in the pattern would prevent this conversion, But unfortunately
> > this will conflict with RVV (RISC-V Vector Extension).
> >
> > This is why I initially considered using UNSPEC to address the XTheadVector
> > problem while minimizing interference with RVV.
> >
> > I'm not sure if there is a better way, do you have any suggestions?
> 
> We'd probably need to disable the alternatives via the "spec_restriction"
> attribute as we do for the fma ops.

Hi, Robin
Thank you very much for your professional reply. I am trying to solve the problem
using the "spec_restriction" way. But unfortunately, I have a new problem. As
pattern below, how can I enable "r" and disable "K" when XTheadVector? "rK" already
seems to be the smallest unit and not able to be
controlled separately using spec_restriction?

(define_insn "@pred_madc<mode>"
  [(set (match_operand:<VM> 0 "register_operand"         "=vr, &vr, &vr")
	(unspec:<VM>
	   [(plus:VI
	     (match_operand:VI 1 "register_operand"     "  %0,  vr,  vr")
	     (match_operand:VI 2 "vector_arith_operand" "vrvi,  vr,  vi"))
	    (match_operand:<VM> 3 "register_operand"    "  vm,  vm,  vm")
	    (unspec:<VM>
	      [(match_operand 4 "vector_length_operand" "  rK,  rK,  rK")
	       (match_operand 5 "const_int_operand"     "   i,   i,   i")
	       (reg:SI VL_REGNUM)
	       (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)] UNSPEC_VMADC))]
  "TARGET_VECTOR"
  "vmadc.v%o2m\t%0,%1,%v2,%3"
  [(set_attr "type" "vicalu")
   (set_attr "mode" "<MODE>")
   (set_attr "vl_op_idx" "4")
   (set (attr "avl_type_idx") (const_int 5))
   (set_attr "spec_restriction" "thv,none,none")])
  
BR
Jin

> --
> Regards
> Robin
>
  

> Thank you very much for your professional reply. I am trying to solve the problem
> using the "spec_restriction" way. But unfortunately, I have a new problem. As
> pattern below, how can I enable "r" and disable "K" when XTheadVector? "rK" already
> seems to be the smallest unit and not able to be
> controlled separately using spec_restriction?
>
> (define_insn "@pred_madc<mode>"
>   [(set (match_operand:<VM> 0 "register_operand"         "=vr, &vr, &vr")
> 	(unspec:<VM>
> 	   [(plus:VI
> 	     (match_operand:VI 1 "register_operand"     "  %0,  vr,  vr")
> 	     (match_operand:VI 2 "vector_arith_operand" "vrvi,  vr,  vi"))
> 	    (match_operand:<VM> 3 "register_operand"    "  vm,  vm,  vm")
> 	    (unspec:<VM>
> 	      [(match_operand 4 "vector_length_operand" "  rK,  rK,  rK")
> 	       (match_operand 5 "const_int_operand"     "   i,   i,   i")
> 	       (reg:SI VL_REGNUM)
> 	       (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)] UNSPEC_VMADC))]
>   "TARGET_VECTOR"
>   "vmadc.v%o2m\t%0,%1,%v2,%3"
>   [(set_attr "type" "vicalu")
>    (set_attr "mode" "<MODE>")
>    (set_attr "vl_op_idx" "4")
>    (set (attr "avl_type_idx") (const_int 5))
>    (set_attr "spec_restriction" "thv,none,none")])

"rK" can be split up further into "r" and "K" so I'd say you
need to adjust (and split) the alternatives accordingly.  The new "r"
alternative would have spec_restriction "none" and the "K" alternative "rvv".
  

> > Thank you very much for your professional reply. I am trying to solve the problem
> > using the "spec_restriction" way. But unfortunately, I have a new problem. As
> > pattern below, how can I enable "r" and disable "K" when XTheadVector? "rK" already
> > seems to be the smallest unit and not able to be
> > controlled separately using spec_restriction?
> >
> > (define_insn "@pred_madc<mode>"
> >   [(set (match_operand:<VM> 0 "register_operand"         "=vr, &vr, &vr")
> >  (unspec:<VM>
> >     [(plus:VI
> >       (match_operand:VI 1 "register_operand"     "  %0,  vr,  vr")
> >       (match_operand:VI 2 "vector_arith_operand" "vrvi,  vr,  vi"))
> >      (match_operand:<VM> 3 "register_operand"    "  vm,  vm,  vm")
> >      (unspec:<VM>
> >        [(match_operand 4 "vector_length_operand" "  rK,  rK,  rK")
> >         (match_operand 5 "const_int_operand"     "   i,   i,   i")
> >         (reg:SI VL_REGNUM)
> >         (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)] UNSPEC_VMADC))]
> >   "TARGET_VECTOR"
> >   "vmadc.v%o2m\t%0,%1,%v2,%3"
> >   [(set_attr "type" "vicalu")
> >    (set_attr "mode" "<MODE>")
> >    (set_attr "vl_op_idx" "4")
> >    (set (attr "avl_type_idx") (const_int 5))
> >    (set_attr "spec_restriction" "thv,none,none")])
> 
> "rK" can be split up further into "r" and "K" so I'd say you
> need to adjust (and split) the alternatives accordingly.  The new "r"
> alternative would have spec_restriction "none" and the "K" alternative "rvv".

Yes. This will solve the problem, but it will lead to very large-scale changes
(splitting each rK, adding 1 column constraint), and make the pattern more complex
and more difficult to maintain. In contrast, how about replacing "rK" with a new
constrain in the way jeff mentioned? For example, "Vvl".

Jeff: We could also create a new constraint that mostly behaves like rK, but 
rejects (const_int 1) when thead-vector is enabled and use that in the 
vsetvl pattern instead of rK. ( https://gcc.gnu.org/pipermail/gcc-patches/2024-December/671836.html )

BR
Jin

> -- 
> Regards
>  Robin
  

> Yes. This will solve the problem, but it will lead to very large-scale changes
> (splitting each rK, adding 1 column constraint), and make the pattern more complex
> and more difficult to maintain. In contrast, how about replacing "rK" with a new
> constrain in the way jeff mentioned? For example, "Vvl".

> Jeff: We could also create a new constraint that mostly behaves like rK, but 
> rejects (const_int 1) when thead-vector is enabled and use that in the 
> vsetvl pattern instead of rK. ( https://gcc.gnu.org/pipermail/gcc-patches/2024-December/671836.html )

If you need to change every rK, then yeah, spec_restriction seem reasonable and
a new constraint seems better.
  

On 1/13/25 1:41 AM, Jin Ma wrote:
>>> Thank you very much for your professional reply. I am trying to solve the problem
>>> using the "spec_restriction" way. But unfortunately, I have a new problem. As
>>> pattern below, how can I enable "r" and disable "K" when XTheadVector? "rK" already
>>> seems to be the smallest unit and not able to be
>>> controlled separately using spec_restriction?
>>>
>>> (define_insn "@pred_madc<mode>"
>>>    [(set (match_operand:<VM> 0 "register_operand"         "=vr, &vr, &vr")
>>>   (unspec:<VM>
>>>      [(plus:VI
>>>        (match_operand:VI 1 "register_operand"     "  %0,  vr,  vr")
>>>        (match_operand:VI 2 "vector_arith_operand" "vrvi,  vr,  vi"))
>>>       (match_operand:<VM> 3 "register_operand"    "  vm,  vm,  vm")
>>>       (unspec:<VM>
>>>         [(match_operand 4 "vector_length_operand" "  rK,  rK,  rK")
>>>          (match_operand 5 "const_int_operand"     "   i,   i,   i")
>>>          (reg:SI VL_REGNUM)
>>>          (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)] UNSPEC_VMADC))]
>>>    "TARGET_VECTOR"
>>>    "vmadc.v%o2m\t%0,%1,%v2,%3"
>>>    [(set_attr "type" "vicalu")
>>>     (set_attr "mode" "<MODE>")
>>>     (set_attr "vl_op_idx" "4")
>>>     (set (attr "avl_type_idx") (const_int 5))
>>>     (set_attr "spec_restriction" "thv,none,none")])
>>
>> "rK" can be split up further into "r" and "K" so I'd say you
>> need to adjust (and split) the alternatives accordingly.  The new "r"
>> alternative would have spec_restriction "none" and the "K" alternative "rvv".
> 
> Yes. This will solve the problem, but it will lead to very large-scale changes
> (splitting each rK, adding 1 column constraint), and make the pattern more complex
> and more difficult to maintain. In contrast, how about replacing "rK" with a new
> constrain in the way jeff mentioned? For example, "Vvl".
> 
> Jeff: We could also create a new constraint that mostly behaves like rK, but
> rejects (const_int 1) when thead-vector is enabled and use that in the
> vsetvl pattern instead of rK. ( https://gcc.gnu.org/pipermail/gcc-patches/2024-December/671836.html )
I think both roughly end up being the same amount of work.  I'm not too 
worried about the extra alternatives and such, risc-v is pretty 
reasonable in that regard if you compare it to some other ports.

Both ultimately end up forcing us to adjust the constraints on every 
affected pattern.  I think that's unavoidable.

jeff