Re: Re: [PATCH 2/3 V2] RISC-V: Enable basic auto-vectorization for RVV

["juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai>]

[-- Attachment #1: Type: text/plain, Size: 5701 bytes --]

>> The comment above might not sync with your implementation?
Address comment.

>> Actually, you've allowed TARGET_MIN_VLEN < 128 && riscv_autovec_lmul < RVV_M2
Not sure I am on the same page with you. I return word_mode for this situation, the auto-vectorization
will be disabled. I have testcase to test it and I didn't see issue. Would you mind giving me more informations?

>> Could you put a gcc_unreachable or assertion here? I assume this
>>should never false?
>>if (!multiple_p (vector_size, scalar_size, &nunits))
>>  {
  >>  gcc_unreachable ();
  >>  return word_mode;
 >> }
ok.

>> ../riscv-gnu-toolchain-trunk/riscv-gcc/gcc/testsuite/gcc.target/riscv/rvv/base/spill-10.c:
>>In function 'stach_check_alloca_1':
>>../riscv-gnu-toolchain-trunk/riscv-gcc/gcc/testsuite/gcc.target/riscv/rvv/base/spill-10.c:41:1:
>>error: insn does not satisfy its constraints:
>>   41 | }
>>      | ^
>>(insn 37 26 40 2 (set (reg:VNx8QI 120 v24 [orig:158 data ] [158])
>>        (reg:VNx8QI 10 a0 [ data ]))
>>"../riscv-gnu-toolchain-trunk/riscv-gcc/gcc/testsuite/gcc.target/riscv/rvv/base/spill-10.c":28:1
>>727 {*movvnx8qi_whole}
 >>    (nil))

Oh, I see. According to your situation, the LMUL = 1 is 128bit. VNx8QImode is MF2 which is 64bit size.
GCC tie VNx8QI into a scalar register. I think it can be easily fixed in the backend but yes, I agree with you we drop
this option at the first time.



juzhe.zhong@rivai.ai
 
From: Kito Cheng
Date: 2023-04-20 10:26
To: juzhe.zhong
CC: gcc-patches; palmer; jeffreyalaw
Subject: Re: [PATCH 2/3 V2] RISC-V: Enable basic auto-vectorization for RVV
> +/* Return the vectorization machine mode for RVV according to LMUL.  */
> +machine_mode
> +preferred_simd_mode (scalar_mode mode)
> +{
> +  /* We only enable auto-vectorization when TARGET_MIN_VLEN >= 128
> +     which is -march=rv64gcv. Since GCC loop vectorizer report ICE
> +     when we enable -march=rv64gc_zve32* and -march=rv32gc_zve64*.
> +     in the 'can_duplicate_and_interleave_p' of tree-vect-slp.cc. Since we have
> +     VNx1SImode in -march=*zve32* and VNx1DImode in -march=*zve64*, they are
> +     enabled in targetm. vector_mode_supported_p and SLP vectorizer will try to
> +     use them. Currently, we can support auto-vectorization in
> +     -march=rv32_zve32x_zvl128b. Wheras, -march=rv32_zve32x_zvl32b or
> +     -march=rv32_zve32x_zvl64b are disabled.
> + */
 
The comment above might not sync with your implementation?
 
> +  if (autovec_use_vlmax_p ())
> +    {
> +      /* If TARGET_MIN_VLEN < 128, we don't allow LMUL < 2
> +        auto-vectorization since Loop Vectorizer may use VNx1SImode or
> +        VNx1DImode to vectorize which will create ICE in the
> +        'can_duplicate_and_interleave_p' of tree-vect-slp.cc.  */
> +      if (TARGET_MIN_VLEN < 128 && riscv_autovec_lmul < RVV_M2)
> +       return word_mode;
 
Actually, you've allowed TARGET_MIN_VLEN < 128 && riscv_autovec_lmul < RVV_M2
 
> +      /* We use LMUL = 1 as base bytesize which is BYTES_PER_RISCV_VECTOR and
> +        riscv_autovec_lmul as multiply factor to calculate the the NUNITS to
> +        get the auto-vectorization mode.  */
> +      poly_uint64 nunits;
> +      poly_uint64 vector_size
> +       = BYTES_PER_RISCV_VECTOR * ((int) riscv_autovec_lmul);
> +      poly_uint64 scalar_size = GET_MODE_SIZE (mode);
> +      if (!multiple_p (vector_size, scalar_size, &nunits))
> +       return word_mode;
 
Could you put a gcc_unreachable or assertion here? I assume this
should never false?
if (!multiple_p (vector_size, scalar_size, &nunits))
  {
    gcc_unreachable ();
    return word_mode;
  }
 
> +      machine_mode rvv_mode;
> +      if (get_vector_mode (mode, nunits).exists (&rvv_mode))
> +       return rvv_mode;
> +    }
> +  /* TODO: We will support minimum length VLS auto-vectorization in the future.
> +   */
> +  return word_mode;
> +}
> +
> +
>  } // namespace riscv_vector
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 5d2550871c7..c601389b540 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -6228,7 +6228,15 @@ riscv_convert_vector_bits (void)
>       to set RVV mode size. The RVV machine modes size are run-time constant if
>       TARGET_VECTOR is enabled. The RVV machine modes size remains default
>       compile-time constant if TARGET_VECTOR is disabled.  */
> -  return TARGET_VECTOR ? poly_uint16 (1, 1) : 1;
> +  if (TARGET_VECTOR)
> +    {
> +      if (riscv_autovec_preference == RVV_FIXED_VLMAX)
> +       return (int) TARGET_MIN_VLEN / (riscv_bytes_per_vector_chunk * 8);
 
I realized this will also effect intrinsic stuffs.
 
So I would prefer to drop RVV_FIXED_VLMAX stuffs at this moment.
 
e.g.
$ riscv64-unknown-linux-gnu-gcc
--param=riscv-autovec-preference=fixed-vlmax
gcc/testsuite/gcc.target/riscv/rvv/base/spill-10.c -O2 -march=rv64gcv
-S
../riscv-gnu-toolchain-trunk/riscv-gcc/gcc/testsuite/gcc.target/riscv/rvv/base/spill-10.c:
In function 'stach_check_alloca_1':
../riscv-gnu-toolchain-trunk/riscv-gcc/gcc/testsuite/gcc.target/riscv/rvv/base/spill-10.c:41:1:
error: insn does not satisfy its constraints:
   41 | }
      | ^
(insn 37 26 40 2 (set (reg:VNx8QI 120 v24 [orig:158 data ] [158])
        (reg:VNx8QI 10 a0 [ data ]))
"../riscv-gnu-toolchain-trunk/riscv-gcc/gcc/testsuite/gcc.target/riscv/rvv/base/spill-10.c":28:1
727 {*movvnx8qi_whole}
     (nil))
during RTL pass: reload
../riscv-gnu-toolchain-trunk/riscv-gcc/gcc/testsuite/gcc.target/riscv/rvv/base/spill-10.c:41:1:
internal compiler error: in extract_constrain_insn, at recog.cc:2692