RE: Re: [PATCH V5] RISC-V: Using merge approach to optimize repeating sequence in vec_init

2023-05-24 Thread Li, Pan2 via Gcc-patches
Hi Kito,

Update the PATCH v6 with refactored framework as below, thanks for comments.

https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619536.html

Pan

-Original Message-
From: Gcc-patches  On Behalf 
Of Kito Cheng via Gcc-patches
Sent: Wednesday, May 17, 2023 11:52 AM
To: juzhe.zh...@rivai.ai
Cc: gcc-patches ; palmer ; 
jeffreyalaw 
Subject: Re: Re: [PATCH V5] RISC-V: Using merge approach to optimize repeating 
sequence in vec_init

On Wed, May 17, 2023 at 11:36 AM juzhe.zh...@rivai.ai  
wrote:
>
> >> Does it means we assume inner_int_mode is DImode? (because sizeof 
> >> (uint64_t)) or it should be something like `for (unsigned int i = 
> >> 0; i < (GET_MODE_SIZE(inner_int_mode ()) * 8 / npatterns ()); i++)` ?
> No, sizeof (uint64_t) means uint64_t mask = 0;

+  return gen_int_mode (mask, inner_int_mode ());
And we expect the uint64_t mask can always be put into inner_int_mode ()?
If not, why do we fill up all 64 bits?

>
> >> Do you mind give more comment about this? what it checked and what it did?
> The reason we use known_gt (GET_MODE_SIZE (dup_mode), 
> BYTES_PER_RISCV_VECTOR) since we want are using vector integer mode to 
> generate the mask for example we generate 0b01010101010101 mask, we 
> should use a scalar register holding value = 0b010101010...
> Then vmv.v.x into a vector,then this vector will be used as a mask.
>
> >> Why this only hide in else? I guess I have this question is because 
> >> I don't fully understand the logic of the if condition?
>
> Since we can't vector floting-point instruction to generate a mask.

I don't get why it's not something like below?

if (known_gt (GET_MODE_SIZE (dup_mode), BYTES_PER_RISCV_VECTOR)) { ...
}
if (FLOAT_MODE_P (dup_mode))
{
...
}



>
> >> nit: builder.inner_mode () rather than GET_MODE_INNER (dup_mode)?
>
> They are the same. I can change it using GET_MODE_INNER
>
> >> And I would like have more commnet to explain why we need force_reg here.
> Since it will creat ICE.

But why? And why can it be resolved by force_reg? you need few more comment in 
the code


Re: Re: [PATCH V5] RISC-V: Using merge approach to optimize repeating sequence in vec_init

2023-05-16 Thread Kito Cheng via Gcc-patches
On Wed, May 17, 2023 at 11:36 AM juzhe.zh...@rivai.ai
 wrote:
>
> >> Does it means we assume inner_int_mode is DImode? (because sizeof 
> >> (uint64_t))
> >> or it should be something like `for (unsigned int i = 0; i <
> >> (GET_MODE_SIZE(inner_int_mode ()) * 8 / npatterns ()); i++)` ?
> No, sizeof (uint64_t) means uint64_t mask = 0;

+  return gen_int_mode (mask, inner_int_mode ());
And we expect the uint64_t mask can always be put into inner_int_mode ()?
If not, why do we fill up all 64 bits?

>
> >> Do you mind give more comment about this? what it checked and what it did?
> The reason we use known_gt (GET_MODE_SIZE (dup_mode), BYTES_PER_RISCV_VECTOR)
> since we want are using vector integer mode to generate the mask for example
> we generate 0b01010101010101 mask, we should use a scalar register 
> holding value = 0b010101010...
> Then vmv.v.x into a vector,then this vector will be used as a mask.
>
> >> Why this only hide in else? I guess I have this question is because I
> >> don't fully understand the logic of the if condition?
>
> Since we can't vector floting-point instruction to generate a mask.

I don't get why it's not something like below?

if (known_gt (GET_MODE_SIZE (dup_mode), BYTES_PER_RISCV_VECTOR))
{
...
}
if (FLOAT_MODE_P (dup_mode))
{
...
}



>
> >> nit: builder.inner_mode () rather than GET_MODE_INNER (dup_mode)?
>
> They are the same. I can change it using GET_MODE_INNER
>
> >> And I would like have more commnet to explain why we need force_reg here.
> Since it will creat ICE.

But why? And why can it be resolved by force_reg? you need few more
comment in the code


Re: Re: [PATCH V5] RISC-V: Using merge approach to optimize repeating sequence in vec_init

2023-05-16 Thread juzhe.zh...@rivai.ai
>> Does it means we assume inner_int_mode is DImode? (because sizeof (uint64_t))
>> or it should be something like `for (unsigned int i = 0; i <
>> (GET_MODE_SIZE(inner_int_mode ()) * 8 / npatterns ()); i++)` ?
No, sizeof (uint64_t) means uint64_t mask = 0;

>> Do you mind give more comment about this? what it checked and what it did?
The reason we use known_gt (GET_MODE_SIZE (dup_mode), BYTES_PER_RISCV_VECTOR)
since we want are using vector integer mode to generate the mask for example
we generate 0b01010101010101 mask, we should use a scalar register holding 
value = 0b010101010...
Then vmv.v.x into a vector,then this vector will be used as a mask.

>> Why this only hide in else? I guess I have this question is because I
>> don't fully understand the logic of the if condition?

Since we can't vector floting-point instruction to generate a mask.

>> nit: builder.inner_mode () rather than GET_MODE_INNER (dup_mode)?

They are the same. I can change it using GET_MODE_INNER

>> And I would like have more commnet to explain why we need force_reg here.
Since it will creat ICE.




juzhe.zh...@rivai.ai
 
From: Kito Cheng
Date: 2023-05-17 11:21
To: juzhe.zhong
CC: gcc-patches; palmer; jeffreyalaw
Subject: Re: [PATCH V5] RISC-V: Using merge approach to optimize repeating 
sequence in vec_init
> +
> +/* Get the mask for merge approach.
> +
> + Consider such following case:
> +   {a, b, a, b, a, b, a, b, a, b, a, b, a, b, a, b}
> + To merge "a", the mask should be 1010
> + To merge "b", the mask should be 0101
> +*/
> +rtx
> +rvv_builder::get_merge_mask_bitfield (unsigned int index) const
> +{
> +  uint64_t base_mask = (1ULL << index);
> +  uint64_t mask = 0;
> +  for (unsigned int i = 0; i < (sizeof (uint64_t) * 8 / npatterns ()); i++)
> +mask |= base_mask << (i * npatterns ());
> +  return gen_int_mode (mask, inner_int_mode ());
 
Does it means we assume inner_int_mode is DImode? (because sizeof (uint64_t))
or it should be something like `for (unsigned int i = 0; i <
(GET_MODE_SIZE(inner_int_mode ()) * 8 / npatterns ()); i++)` ?
 
> +}
> +
>  /* Subroutine of riscv_vector_expand_vector_init.
> Works as follows:
> (a) Initialize TARGET by broadcasting element NELTS_REQD - 1 of BUILDER.
> @@ -1226,6 +1307,107 @@ expand_vector_init_insert_elems (rtx target, const 
> rvv_builder &builder,
>  }
>  }
>
> +/* Emit vmv.s.x instruction.  */
> +
> +static void
> +emit_scalar_move_op (rtx dest, rtx src, machine_mode mask_mode)
> +{
> +  insn_expander<8> e;
> +  machine_mode mode = GET_MODE (dest);
> +  rtx scalar_move_mask = gen_scalar_move_mask (mask_mode);
> +  e.set_dest_and_mask (scalar_move_mask, dest, mask_mode);
> +  e.add_input_operand (src, GET_MODE_INNER (mode));
> +  e.set_len_and_policy (const1_rtx, false);
> +  e.expand (code_for_pred_broadcast (mode), false);
> +}
> +
> +/* Emit merge instruction.  */
> +
> +static void
> +emit_merge_op (rtx dest, rtx src1, rtx src2, rtx mask)
> +{
> +  insn_expander<8> e;
> +  machine_mode mode = GET_MODE (dest);
> +  e.set_dest_merge (dest);
> +  e.add_input_operand (src1, mode);
> +  if (VECTOR_MODE_P (GET_MODE (src2)))
> +e.add_input_operand (src2, mode);
> +  else
> +e.add_input_operand (src2, GET_MODE_INNER (mode));
> +
> +  e.add_input_operand (mask, GET_MODE (mask));
> +  e.set_len_and_policy (NULL_RTX, true, true, false);
> +  if (VECTOR_MODE_P (GET_MODE (src2)))
> +e.expand (code_for_pred_merge (mode), false);
> +  else
> +e.expand (code_for_pred_merge_scalar (mode), false);
> +}
> +
> +/* Use merge approach to initialize the vector with repeating sequence.
> + v = {a, b, a, b, a, b, a, b}.
> +
> + v = broadcast (a).
> + mask = 0b01010101
> + v = merge (v, b, mask)
> +*/
> +static void
> +expand_vector_init_merge_repeating_sequence (rtx target,
> +const rvv_builder &builder)
> +{
> +  machine_mode mask_mode = get_mask_mode (builder.mode ()).require ();
> +  machine_mode dup_mode = builder.mode ();
> +  if (known_gt (GET_MODE_SIZE (dup_mode), BYTES_PER_RISCV_VECTOR))
> +{
> +  poly_uint64 nunits
> +   = exact_div (BYTES_PER_RISCV_VECTOR, builder.inner_units ());
> +  dup_mode = get_vector_mode (builder.inner_int_mode (), nunits).require 
> ();
> +}
 
Do you mind give more comment about this? what it checked and what it did?
 
> +  else
> +{
> +  if (FLOAT_MODE_P (dup_mode))
> +   {
> + poly_uint64 nunits = GET_MODE_NUNITS (dup_mode);
> + dup_mode
> +   = get_vector_mode (builder.inner_int_mode (), nunits).require ();
> +   }
 
Why this only hide in else? I guess I have this question is because I
don't fully understand the logic of the if condition?
 
> +}
> +
> +  machine_mode dup_mask_mode = get_mask_mode (dup_mode).require ();
> +
> +  /* Step 1: Broadcast the 1st-pattern.  */
> +  emit_len_op (code_for_pred_broadcast (builder.mode ()), target,
> +  force_reg (builder.in