[PATCH] RISC-V: Refactor and clean emit_{vlmax, nonvlmax}_xxx functions
Lehua Ding
lehua.ding@rivai.ai
Thu Aug 31 08:59:18 GMT 2023
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
More information about the Gcc-patches
mailing list