Re: [PATCH v4 05/10] RISC-V:autovec: Add autovectorization patterns for binary integer operations

[Jeff Law <jeffreyalaw@gmail.com>]

On 4/17/23 12:36, Michael Collison wrote:
> 2023-03-02  Michael Collison  <collison@rivosinc.com>
> 	    Juzhe Zhong  <juzhe.zhong@rivai.ai>
> 
> 	* config/riscv/riscv.md (riscv_vector_preferred_simd_mode): Include
> 	vector-iterators.md.
> 	* config/riscv/vector-auto.md: New file containing
> 	autovectorization patterns.
> 	* config/riscv/vector-iterators.md (UNSPEC_VADD/UNSPEC_VSUB):
> 	New unspecs for autovectorization patterns.
> 	* config/riscv/vector.md: Remove include of vector-iterators.md
> 	and include vector-auto.md.
So the basic idea here appears to be to have a define_expand with the 
well known names (for the optab interface) generate RTL that is 
subsequently matched by the intrinsics that Juzhe has already defined 
and integrated.

That seems like a reasonable model to start with and get the basic 
functionality in place.  I'm all for focusing on that basic 
functionality first.


> diff --git a/gcc/config/riscv/vector-auto.md b/gcc/config/riscv/vector-auto.md
> new file mode 100644
> index 00000000000..dc62f9af705
> --- /dev/null
> +++ b/gcc/config/riscv/vector-auto.md
So basically vector-auto.md provides the interface to utilize the 
builtins found in vector.md.  Given the size of vector.md I can 
certainly see the desire to separate that out.


> +
> +
> +;; -------------------------------------------------------------------------
> +;; ---- [INT] Addition
Just a note.  This patch actually wires up plus, minus, and, ior, xor, 
ashift, ashiftrt and lshiftrt.  So it's quite a bit more than just 
addition.  So updating the comments is probably warranted.


> +;; -------------------------------------------------------------------------
> +;; Includes:
> +;; - vadd.vv
> +;; - vadd.vx
> +;; - vadd.vi
> +;; -------------------------------------------------------------------------
> +
> +(define_expand "<optab><mode>3"
> +  [(set (match_operand:VI 0 "register_operand")
> +	(any_int_binop:VI (match_operand:VI 1 "register_operand")
> +			  (match_operand:VI 2 "register_operand")))]
> +  "TARGET_VECTOR"
> +{
> +  using namespace riscv_vector;
> +
> +  rtx merge = RVV_VUNDEF (<MODE>mode);
> +  rtx vl = gen_reg_rtx (Pmode);
> +  emit_vlmax_vsetvl (<MODE>mode, vl);
> +  rtx mask_policy = get_mask_policy_no_pred();
> +  rtx tail_policy = get_tail_policy_no_pred();
> +  rtx mask = CONSTM1_RTX(<VM>mode);
> +  rtx vlmax_avl_p = get_avl_type_rtx(NONVLMAX);
> +
> +  emit_insn(gen_pred_<optab><mode>(operands[0], mask, merge, operands[1], operands[2],
> +				vl, tail_policy, mask_policy, vlmax_avl_p));
Just nits.  Make sure to put a space before the open paren of an 
argument list, even when the argument list is empty.  Similarly for the 
other expander in here.  And update the comment.  You may not want to 
list every instruction handled by the expander.  Your call, though 
clearly if you're going to include them, the list ought to be reasonably 
complete.

No objections to this code.  It obviously depends on some bits earlier 
in the patchset which I still need to look at, but I wanted to look at 
this one first as it shows the basic formula for how to wire up the 
basic vector patterns.

Please wait for the prereqs to get reviewed before installing on the trunk.

jeff