Re: [PATCH] RISC-V: Refactor and clean emit_{vlmax, nonvlmax}_xxx functions

[Lehua Ding <lehua.ding@rivai.ai>]

Committed after addressing these comments.
Many thanks to Kito for taking the time to review such a large patch :)

On 2023/8/31 15:51, Kito Cheng wrote:
> Thanks for the cleanup, the new interface is really much simpler than before!
> 
> Only few minor comment, you can go ahead to commit that after address
> those comment.
> 
> (OK, I don't want to review whole patch again, it's really huge change :P
> 
>>
>> -  void set_rounding_mode (enum floating_point_rounding_mode mode)
>> +  void check_insn_flags ()
> 
> void check_insn_flags () const
> 
>>     void emit_insn (enum insn_code icode, rtx *ops)
>>     {
>>       int opno = 0;
>> +    int num_ops = 0;
>>       /* It's true if any operand is memory operand.  */
>>       bool any_mem_p = false;
>> -    /* It's true if all operands are mask operand.  */
>> -    bool all_mask_p = true;
>> -    if (m_has_dest_p)
>> +
>> +    /* Add dest operand.  */
>> +    if (m_insn_flags & HAS_DEST_P)
>>         {
>>          any_mem_p |= MEM_P (ops[opno]);
>> -       all_mask_p &= GET_MODE_CLASS (GET_MODE (ops[opno])) == MODE_VECTOR_BOOL;
>>          add_output_operand (ops[opno++], m_dest_mode);
>> +       num_ops += 1;
> 
> Drop this
> 
>>         }
>>
>> -    if (m_fully_unmasked_p)
>> +    /* Add mask operand.  */
>> +    if (m_insn_flags & USE_ONE_TRUE_MASK_P)
>> +      add_input_operand (gen_scalar_move_mask (m_mask_mode), m_mask_mode);
>> +    else if (m_insn_flags & USE_ALL_TRUES_MASK_P)
>>         add_all_one_mask_operand ();
>> +    else if (m_insn_flags & HAS_MASK_P)
>> +      {
>> +       machine_mode mode = insn_data[(int) icode].operand[m_opno].mode;
>> +       gcc_assert (mode != VOIDmode);
>> +       add_input_operand (ops[opno++], mode);
>> +       num_ops += 1;
>> +      }
> 
> Drop this
> 
>> -    if (!m_use_real_merge_p)
>> +    /* Add merge operand.  */
>> +    if (m_insn_flags & USE_VUNDEF_MERGE_P)
>>         add_vundef_operand ();
>> +    else if (m_insn_flags & HAS_MERGE_P)
>> +      {
>> +       machine_mode mode = insn_data[(int) icode].operand[m_opno].mode;
>> +       gcc_assert (mode != VOIDmode);
>> +       add_input_operand (ops[opno++], mode);
>> +       num_ops += 1;
>> +      }
>> +
>> +    if (m_insn_flags & NULLARY_OP_P)
>> +      num_ops += 0;
>> +    else if (m_insn_flags & UNARY_OP_P)
>> +      num_ops += 1;
>> +    else if (m_insn_flags & BINARY_OP_P)
>> +      num_ops += 2;
>> +    else if (m_insn_flags & TERNARY_OP_P)
>> +      num_ops += 3;
> 
> num_ops = rather than += here.
> 
>> +    else
>> +      gcc_unreachable ();
>>
>> -    for (; opno < m_op_num; opno++)
>> +    /* Add the remain operands.  */
>> +    for (; opno < num_ops; opno++)
> 
> for (;num_ops;num_ops--, opno++)
> 
>>         {
>>          any_mem_p |= MEM_P (ops[opno]);
>> -       all_mask_p &= GET_MODE_CLASS (GET_MODE (ops[opno])) == MODE_VECTOR_BOOL;
>>          machine_mode mode = insn_data[(int) icode].operand[m_opno].mode;
>>          /* 'create_input_operand doesn't allow VOIDmode.
>>             According to vector.md, we may have some patterns that do not have
>> @@ -194,46 +251,51 @@ public:
>>          add_input_operand (ops[opno], mode);
>>         }
>>
>> -    if (m_needs_avl_p)
>> +    /* Add vl operand.  */
>> +    rtx len = m_vl_op;
>> +    machine_mode mode = VECTOR_MODE_P (m_dest_mode) ? m_dest_mode : m_mask_mode;
>> +    if (m_vlmax_p)
>>         {
>> -       rtx len = m_vl_op;
>> -       machine_mode mode
>> -         = VECTOR_MODE_P (m_dest_mode) ? m_dest_mode : m_mask_mode;
>> -       if (m_vlmax_p)
>> +       if (riscv_v_ext_vls_mode_p (mode))
>> +         {
>> +           /* VLS modes always set VSETVL by
>> +              "vsetvl zero, rs1/imm".  */
>> +           poly_uint64 nunits = GET_MODE_NUNITS (mode);
>> +           len = gen_int_mode (nunits, Pmode);
>> +           if (!satisfies_constraint_K (len))
>> +             len = force_reg (Pmode, len);
>> +           m_vlmax_p = false;
>> +         }
>> +       else if (const_vlmax_p (mode))
>> +         {
>> +           /* Optimize VLS-VLMAX code gen, we can use vsetivli instead of
>> +              the vsetvli to obtain the value of vlmax.  */
>> +           poly_uint64 nunits = GET_MODE_NUNITS (mode);
>> +           len = gen_int_mode (nunits, Pmode);
>> +           m_vlmax_p = false;
>> +         }
>> +       else if (can_create_pseudo_p ())
>>            {
>> -           if (riscv_v_ext_vls_mode_p (mode))
>> -             {
>> -               /* VLS modes always set VSETVL by
>> -                  "vsetvl zero, rs1/imm".  */
>> -               poly_uint64 nunits = GET_MODE_NUNITS (mode);
>> -               len = gen_int_mode (nunits, Pmode);
>> -               if (!satisfies_constraint_K (len))
>> -                 len = force_reg (Pmode, len);
>> -               m_vlmax_p = false; /* It has became NONVLMAX now.  */
>> -             }
>> -           else if (const_vlmax_p (mode))
>> -             {
>> -               /* Optimize VLS-VLMAX code gen, we can use vsetivli instead of
>> -                  the vsetvli to obtain the value of vlmax.  */
>> -               poly_uint64 nunits = GET_MODE_NUNITS (mode);
>> -               len = gen_int_mode (nunits, Pmode);
>> -               m_vlmax_p = false; /* It has became NONVLMAX now.  */
>> -             }
>> -           else if (can_create_pseudo_p ())
>> -             {
>> -               len = gen_reg_rtx (Pmode);
>> -               emit_vlmax_vsetvl (mode, len);
>> -             }
>> +           len = gen_reg_rtx (Pmode);
>> +           emit_vlmax_vsetvl (mode, len);
>>            }
>> -       add_input_operand (len, Pmode);
>> +       else
>> +         gcc_assert (len != NULL_RTX);
> 
> I don't see the gcc_assert here before, and seems like that should
> move to another place
> 
>>         }
>> +    else
>> +      /* Must specify vl when not vlmax.  */
>> +      gcc_assert (len != NULL_RTX);
> 
> Like here and drop the else since we need this assert no matter
> m_vlmax_p true or false
> 
> 
> diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
> index 3e7caf48742..64e22177e81 100644
> --- a/gcc/config/riscv/riscv-v.cc
> +++ b/gcc/config/riscv/riscv-v.cc
> @@ -279,12 +279,8 @@ public:
>              len = gen_reg_rtx (Pmode);
>              emit_vlmax_vsetvl (mode, len);
>            }
> -       else
> -         gcc_assert (len != NULL_RTX);
>         }
> -    else
> -      /* Must specify vl when not vlmax.  */
> -      gcc_assert (len != NULL_RTX);
> +    gcc_assert (len != NULL_RTX);
>       add_input_operand (len, Pmode);
> 
>       /* Add tail and mask policy operands.  */
> 
> 
>> @@ -1212,7 +834,7 @@ emit_vlmax_masked_gather_mu_insn (rtx target, rtx op, rtx sel, rtx mask)
>>        p q r s t u v w  # v11 destination register
>>             e d c b a  # v1 source vector
>>        1 0 0 1 1 1 0 1  # v0 mask vector
>> -
>> +make
> 
> Ha, this must be a mistake, right? :P
> 

-- 
Best,
Lehua