Re: [PATCH v2 2/3] RISC-V: setmem for RISCV with V extension

[Jeff Law <jeffreyalaw@gmail.com>]

On 12/19/23 02:53, Sergei Lewis wrote:
> gcc/ChangeLog
> 
>      * config/riscv/riscv-protos.h (riscv_vector::expand_vec_setmem): New function
>      declaration.
> 
>      * config/riscv/riscv-string.cc (riscv_vector::expand_vec_setmem): New
>      function: this generates an inline vectorised memory set, if and only if we
>      know the entire operation can be performed in a single vector store
> 
>      * config/riscv/riscv.md (setmem<mode>): Try riscv_vector::expand_vec_setmem
>      for constant lengths
> 
> gcc/testsuite/ChangeLog
>      * gcc.target/riscv/rvv/base/setmem-1.c: New tests
>      * gcc.target/riscv/rvv/base/setmem-2.c: New tests
>      * gcc.target/riscv/rvv/base/setmem-3.c: New tests
As with patch 1/3 this needs to be regression tested.  The other 
concern, which I should have voiced with patch 1/3 is that this was 
submitted after the gcc-14 development window closed.  While we do have 
some degrees of freedom to accept backend specific new features, we 
really shouldn't be adding new features/optimizations at this point.  We 
really should just be fixing bugs and new features should be queued for 
gcc-15.




> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> index 1b3f66fd15c..dd34211ca80 100644
> --- a/gcc/config/riscv/riscv.md
> +++ b/gcc/config/riscv/riscv.md
> @@ -2387,6 +2387,20 @@
>       FAIL;
>   })
>   
> +(define_expand "setmemsi"
> +  [(set (match_operand:BLK 0 "memory_operand")     ;; Dest
> +	      (match_operand:QI  2 "nonmemory_operand")) ;; Value
> +   (use (match_operand:SI  1 "const_int_operand")) ;; Length
> +   (match_operand:SI       3 "const_int_operand")] ;; Align
> +  "TARGET_VECTOR"
> +{
> +  if (riscv_vector::expand_vec_setmem (operands[0], operands[1], operands[2],
> +      operands[3]))
> +    DONE;
> +  else
> +    FAIL;
> +})
Is the :SI really needed for operands1 and operands3?  a CONST_INT node 
never has a mode.    Or is the existence of the mode just to keep the 
gen* programs from generating a warning?  And if we're going to keep a 
mode, particularly on the length, shouldn't the length be in mode P?


Jeff